From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 6 Jun 2019 13:04:01 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 01/10] box: zap atfork callback Message-ID: <20190606100401.xwgtkfw5rh5c5odu@esperanza> References: <04a7e79410d5590679cacf9f917c3da562d6a51f.1558103547.git.vdavydov.dev@gmail.com> <20190518183719.GB9448@atlas> <20190601081608.GB29429@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190601081608.GB29429@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Sat, Jun 01, 2019 at 11:16:08AM +0300, Konstantin Osipov wrote: > * Konstantin Osipov [19/05/18 21:41]: > > * Vladimir Davydov [19/05/17 17:54]: > > > box_atfork calls wal_atfork which in turn calls xlog_atfork for the wal > > > and vylog files. A comment to xlog_atfork says that it's necessary to > > > prevent atexit handlers in a child from closing xlog files again, but we > > > don't use atexit for that anymore. A comment to box_atfork says that > > > box.coredump forks to write a core, but there's no box.coredump anymore. > > > There's also a comment mentioning box.cfg.background, but when we fork > > > that early there's no xlog file open. > > > > > > To sum it up, atfork looks like a piece of legacy code. Let's get rid of > > > it now so as not to bother patching it later. > > > > If anyone adds a fork any time in the future, xlogs will break > > silently, and it would be hard to catch in a test. > > > > If you wish to remove the dead code, please keep the atfork, but panic in it. > > As discussed in the channel, you can't panic in atfork handler > since there is an upcoming popen() patch. This all still looks > very fragile. Note, I didn't remove atfork handlers completely. I only removed the box part, which closes xlog file descriptors. This doesn't make much sense anyway, because there are lots of other file descriptors that need to be closed (think of vinyl) so closing xlog and vylog only is just a half measure. We should probably use O_CLOEXEC instead. That's why I don't understand why you're so opposed to this particular patch. > I read from vfork() man page that NPTL doesn't call > pthread_atfork() handlers in vfork, > while older implementation may. What is the behaviour on freebsd? > If both systems don't invoke the handlers, I would keep the > panic() call, otherwise remove. Hmm, turns out we use fork() to spawn a pipe logger thread. And it may be called after box.cfg from a C module to create a custom logger. So we definitely can't panic() there.