notifier: respawn workers if they're all dead #26
Conversation
kyrylo
force-pushed
the
respawn-workers
branch
2 times, most recently
from
e42b3f2 to
c85cbbf
Compare
|
|
||
| config.workers.times { @workers.add(spawn_worker) } | ||
| @workers.enclose | ||
| spawn_workers |
There was a problem hiding this comment.
Remove this? It is kinda nice that we don't spawn any workers if async notifier is not used.
|
Can you instead check that thread was forked and respawn workers? |
kyrylo
force-pushed
the
respawn-workers
branch
2 times, most recently
from
116f9cb to
fefd305
Compare
|
PTAL |
| @@ -42,6 +44,12 @@ def initialize(user_config) | |||
| ## | |||
| # @macro see_public_api_method | |||
| def notify(exception, params = {}) | |||
| if !@workers_spawned || @pid != Process.pid | |||
There was a problem hiding this comment.
Do you really need !@workers_spawned check? I think @pid != Process.pid should be enough...
There was a problem hiding this comment.
No, with the current refactoring we don't need it.
kyrylo
force-pushed
the
respawn-workers
branch
2 times, most recently
from
992a0b3 to
6f80830
Compare
|
PTAL |
| if @pid != Process.pid && @workers.list.empty? | ||
| @pid = Process.pid | ||
| spawn_workers | ||
| end |
There was a problem hiding this comment.
I think you should not spawn any workers if notifier is closed.
There was a problem hiding this comment.
Should this be enough, then?
diff --git a/lib/airbrake-ruby/async_sender.rb b/lib/airbrake-ruby/async_sender.rb
index 35b1aa4..dc3a71d 100644
--- a/lib/airbrake-ruby/async_sender.rb
+++ b/lib/airbrake-ruby/async_sender.rb
@@ -74,7 +74,7 @@ module Airbrake
def has_workers?
if @pid != Process.pid && @workers.list.empty?
@pid = Process.pid
- spawn_workers
+ spawn_workers unless closed?
end
!@closed && @workers.list.any?
There was a problem hiding this comment.
Also works. My thinking was that in this case @pid won't be updated, but since the sender is closed it doesn't matter. Updated according to your suggestion.
Potentially fixes airbrake/airbrake#463 (falling back to sync delivery because there are no running async workers) I noticed I can reliably reproduce this message when I invoke the Rails console and manually send a notice via `Airbrake.notify`. It happens because by default Rails launches [Spring][1], which preloads the app by using the `fork()` system call. And when that happens, as we know, our worker threads die. The fix is simply to respawn workers. It works reliably with the Rails console and I assume it will fix the above mentioned issue. [1]: https://github.com/rails/spring/
kyrylo
force-pushed
the
respawn-workers
branch
from
6f80830 to
ff88d3f
Compare
|
PTAL |
|
LGTM |
notifier: respawn workers if they're all dead
Potentially fixes airbrake/airbrake#463
(falling back to sync delivery because there are no running async
workers)
I noticed I can reliably reproduce this message when I invoke the Rails
console and manually send a notice via
Airbrake.notify. It happensbecause by default Rails launches Spring, which preloads the app
by using the
fork()system call. And when that happens, as we know,our worker threads die.
The fix is simply to respawn workers. It works reliably with the Rails
console and I assume it will fix the above mentioned issue.