Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLOSED] Fix errors log buffer lost #690

Closed
wants to merge 2 commits into from
Closed

[CLOSED] Fix errors log buffer lost #690

wants to merge 2 commits into from

Conversation

dhopkalo
Copy link
Contributor

@dhopkalo dhopkalo commented May 26, 2021

When I created my own worker process and use the code example for the push error to the rr server for workers

    } catch (\Throwable $e) {
        $worker->getWorker()->error((string)$e);
    }

I don't see my original error only the next errors:

2021-05-26T16:35:46.711+0300    DEBUG   server          server/plugin.go:225    worker destructed       {"pid": 24906}
2021-05-26T16:35:46.725+0300    ERROR   server          server/plugin.go:238    worker_watcher_wait: signal: killed; process_wait: signal: killed
github.com/spiral/roadrunner/v2/plugins/server.(*Plugin).collectEvents
        github.com/spiral/roadrunner/v2@v2.2.1/plugins/server/plugin.go:238
github.com/spiral/roadrunner/v2/pkg/events.(*HandlerImpl).Push
        github.com/spiral/roadrunner/v2@v2.2.1/pkg/events/general.go:37
github.com/spiral/roadrunner/v2/pkg/worker_watcher.(*workerWatcher).wait
        github.com/spiral/roadrunner/v2@v2.2.1/pkg/worker_watcher/worker_watcher.go:235
github.com/spiral/roadrunner/v2/pkg/worker_watcher.(*workerWatcher).addToWatch.func1
        github.com/spiral/roadrunner/v2@v2.2.1/pkg/worker_watcher/worker_watcher.go:260
2021-05-26T16:35:47.278+0300    DEBUG   server          server/plugin.go:223    worker constructed      {"pid": 24965}

I think that process killed before error buffer has been clean.

I also find out that http.pool.debug=true make it works ok but it not good idea for the production proposal

then I try to search for cracking changes and find out that version < 2.0.2 work ok but 2.0.3+ have this bug

I did code diff between this two version

and find out that it added some if that kill a worker process that has the state not equal to worker.StateReady
but in my case when I send an error invocation to the push, it has worker.stateWorking and if we just kill it I think it lost all the output date before it can be flushed to the main rr stderr or stdout pipes.

#691

Dmitriy Hopkalo added 2 commits May 27, 2021 00:25
… and server kill worker process before flush the error massage to master stderr/stdout
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #690 (af4d36d) into master (8237737) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #690   +/-   ##
=======================================
  Coverage   68.81%   68.81%           
=======================================
  Files          80       80           
  Lines        3794     3794           
=======================================
  Hits         2611     2611           
  Misses        838      838           
  Partials      345      345           
Impacted Files Coverage Δ
pkg/worker_watcher/worker_watcher.go 85.71% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8237737...af4d36d. Read the comment docs.

@wolfy-j wolfy-j requested a review from rustatian May 27, 2021 07:05
@rustatian
Copy link
Member

Hey @drefixs. Thanks for the PR.

  1. Sorry, but this is not a solution to the described problem. It's ok, that this solution works for you, but imagine a case when a worker can't be stopped by our controlled stop signal because it's frozen. This will lead to zombie workers who will always be in the working state. So, just replacing Kill with Stop will not work here. I need to think about how to solve your problem gracefully. Also, this is a question, why the worker on Push operations was still in the working state.
  2. Why you ignored the PR template?

@rustatian rustatian closed this May 27, 2021
@rustatian rustatian changed the title Fix errors log buffer lost [CLOSED] Fix errors log buffer lost May 27, 2021
@rustatian rustatian added the C-future-compatibility Category: future compatibility label May 27, 2021
@rustatian rustatian requested review from rustatian and removed request for rustatian May 27, 2021 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-future-compatibility Category: future compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants