Project

General

Profile

Refactor #22354

Make SilencedLogger thread safe

Added by Lukas Zapletal almost 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Logging
Target version:

Description

Rails logger silencer code was thread safe for some time:

https://github.com/rails/rails/commit/629efb605728b31ad9644f6f0acaf3760b641a29

But this has been refactored in Rails 5.0 and thread safety is now part of logger instead of silencer:

https://github.com/rails/rails/commit/2518bda97cbbcb33dc9a92e70d5b01c09e64d12d

The patch made assumption that all Rails applications are using Rails logging stack, but that's not the case for Foreman - we rely on much more flexible "logging" gem (https://github.com/TwP/logging) which does not implement this type of thread safety for levels.

Foreman has its own logging delegator called SilencedLogger and the code there was never thread safe, concurrent sprockets requests are now using silence method which is not thread safe at all, with logging gem this leads to level set to ERROR (the default) after few requests in development mode which renders Foreman unusable (no debug logging output). This patch uses the same technique and stores temporary levels into thread-only context, so on concurrent access there are no incorrect readouts.


Related issues

Is duplicate of Foreman - Bug #21870: app logger dies when an exception is thrownDuplicate2017-12-05

Associated revisions

Revision cb09f0de (diff)
Added by Lukas Zapletal over 2 years ago

Fixes #22354 - Make SilencedLogger thread safe

Revision 0d2f0a0c (diff)
Added by Ivan Necas over 2 years ago

Refs #22354 - Don't instance_eval at every instance creation

Before this patch, we were defining the methods at every instance
creation. This means potential performance issues, as defining new
methods causes the method lookup cache to get dropped.

I needed to move the time for loading the silencer a bit later so that
the `Logging::LEVELS` are already initialized (as they are empty at the
beginning).

History

#1 Updated by The Foreman Bot almost 3 years ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/5197 added

#2 Updated by Lukas Zapletal almost 3 years ago

  • Legacy Backlogs Release (now unused) set to 296

#3 Updated by Lukas Zapletal over 2 years ago

  • % Done changed from 0 to 100
  • Status changed from Ready For Testing to Closed

#4 Updated by The Foreman Bot over 2 years ago

  • Pull request https://github.com/theforeman/foreman/pull/5252 added

#5 Updated by The Foreman Bot over 2 years ago

  • Pull request https://github.com/theforeman/foreman/pull/5260 added

#6 Updated by Tomer Brisker over 2 years ago

  • Is duplicate of Bug #21870: app logger dies when an exception is thrown added

Also available in: Atom PDF