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

[2.x] Closes monolog handlers by event listener if the worker stops #854

Merged
merged 10 commits into from
Mar 22, 2024
Merged

[2.x] Closes monolog handlers by event listener if the worker stops #854

merged 10 commits into from
Mar 22, 2024

Conversation

NiroDeveloper
Copy link
Contributor

Fix for #850
Laravel uses Monolog for logging, some monolog handler have a important close() function.
Octane does never call this function! In normal laravel applications, monolog calls close by itself via __destruct().
It seams like __destruct() is never called in Swoole, so close the handler manually per event.

@driesvints driesvints changed the title Fix #850: Close monolog handlers by event listener if the worker stops [2.x] Fix #850: Close monolog handlers by event listener if the worker stops Mar 15, 2024
@taylorotwell taylorotwell marked this pull request as draft March 15, 2024 14:10
@NiroDeveloper NiroDeveloper marked this pull request as ready for review March 18, 2024 11:42
@taylorotwell taylorotwell marked this pull request as draft March 21, 2024 19:09
@taylorotwell
Copy link
Member

Drafting until @nunomaduro can review.

@nunomaduro nunomaduro marked this pull request as ready for review March 22, 2024 16:19
@nunomaduro nunomaduro changed the title [2.x] Fix #850: Close monolog handlers by event listener if the worker stops [2.x] Closes monolog handlers by event listener if the worker stops Mar 22, 2024
@taylorotwell
Copy link
Member

@NiroDeveloper how did you decide it should be placed on WorkerStopping? Why not OperationTerminated?

@NiroDeveloper
Copy link
Contributor Author

The handler should be reused for every request / operation, so it should just call close() at the end of the worker process.
There is already a FlushMonologState listener that calls reset() on the handler between every request, this is the correct behavior as descripted by monolog.

@taylorotwell taylorotwell merged commit 14a0eca into laravel:2.x Mar 22, 2024
12 checks passed
@taylorotwell
Copy link
Member

Thanks @NiroDeveloper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants