-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Close file handle after each write #1883
Conversation
Thank you for making this fix, happy to share it's working for us. We deployed this fix to production today and its resolved hangups we were seeing on |
There is #1882 for 2.x, but the problem is nobody has confirmed this fixes anything for them so far, so I'd rather not merge this for nothing as it does introduce overhead for everyone. |
Unfortunately we have the same error despite the change. |
For reference this is how we applied the workaround directly without needing upstream changes. The project in question is on monolog 1. class ClosingStreamHandler extends StreamHandler
{
protected function streamWrite($stream, array $record)
{
parent::streamWrite($stream, $record);
// https://github.com/Seldaek/monolog/pull/1883
parent::close();
}
} |
I have to correct myself. Unfortunately, we have the error twice in our system. Once through monolog and once via symfony/process. I can confirm that the fix here fixes the monolog case. Thank you for the work and @ivanmartinvalle for the information that helped me understand our case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix is working for us.
Looking at this again, I don't think I want to merge as is.. Maybe it would be ok if we can detect the fwrite failure and reopen the file then, instead of closing every time. That'd have less impact on users that are not affected by the problem. |
See #1882 for the new version of the patch with the retry, I think that should help still, and I'm a lot more comfortable merging this. |
Closing in favor of #1882 which is now merged in 2.x-dev and 3.x-dev |
refs #1862, refs #1860, refs #1634