Page MenuHomePhabricator

Logging: provide a way to safely disable logging in a forked process
ClosedPublic

Authored by labath on Oct 15 2017, 2:42 PM.

Details

Summary

We had a bug where if we had forked (in the ProcessLauncherPosixFork)
while another thread was writing a log message, we would deadlock. This
happened because the fork child inherited the locked log rwmutex, which
would never get unlocked. This meant the child got stuck trying to
disable all log channels.

The bug existed for a while but only started being apparent after
D37930, which started using ThreadLauncher (which uses logging) instead
of std::thread (which does not) for launching TaskPool threads.

The fix is to provide a lock free way to disable logging, which should
be used in this case, and this case only.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Oct 15 2017, 2:42 PM

Who is holding the lock and then fork'ing? Seems like we should fix that? I thought all log calls should be "lock, log, unlock". Why is this causing problems?

Never mind, re-reading your original comment made it clear.

eugene edited edge metadata.Oct 16 2017, 1:48 PM

DisableUnlocked methods makes me uneasy.
Maybe we can take a log lock before forking and then properly release it in both parent and child processes?

labath updated this revision to Diff 119315.Oct 17 2017, 6:57 AM

Solve the problem using pthread_atfork().

This way the locks are properly taken before forking and released in both child
and parent processes. This behavior is encapsulated within the Log class and is
completely transparent to all fork users (but it requires a tiny bit of
system-specific code).

clayborg accepted this revision.Oct 17 2017, 10:09 AM

Much better.

This revision is now accepted and ready to land.Oct 17 2017, 10:09 AM
eugene accepted this revision.Oct 17 2017, 9:21 PM
This revision was automatically updated to reflect the committed changes.
labath reopened this revision.Oct 20 2017, 2:13 PM
This revision is now accepted and ready to land.Oct 20 2017, 2:13 PM
labath updated this revision to Diff 119710.Oct 20 2017, 2:13 PM

The previous fix had a problem where a more paraniod libc (e.g., android, but I
suspect freebds would do that as well) would detect that the
thread unlocking the mutex is not the same one as the one that locked, and
refused to comply.

The new fix is a combination of the first two solutions: we disable the logging
via custom code after forking, but we still do it via pthread_atfork to avoid
this detail leaking outside the Log implementation. The added benefit is that
forking code does not need to worry about logging at all, as it will be
automatically disabled.

labath requested review of this revision.Oct 20 2017, 2:13 PM
labath edited edge metadata.
eugene accepted this revision.Oct 23 2017, 10:08 AM
eugene added inline comments.
source/Utility/Log.cpp
333 ↗(On Diff #119710)

A little comment here describing nature of a deadlock if we don't disable logging would be nice.

This revision is now accepted and ready to land.Oct 23 2017, 10:08 AM
labath marked an inline comment as done.Oct 23 2017, 12:41 PM
This revision was automatically updated to reflect the committed changes.