Re: [tarantool-patches] Re: [PATCH 01/10] box: zap atfork callback

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sat, Jun 01, 2019 at 11:16:08AM +0300, Konstantin Osipov wrote:
> * Konstantin Osipov <kostja@tarantool.org> [19/05/18 21:41]:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [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.