search

Seldaek / monolog

Sends your logs to files, sockets, inboxes, databases and various web services
https://seldaek.github.io/monolog/
MIT License
21.05k stars 1.9k forks source link

Close file handle after each write #1883

Closed Seldaek closed 2 weeks ago

Seldaek commented 7 months ago

refs #1862, refs #1860, refs #1634

rossmckelvie commented 6 months ago

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 fwrite() leading to requests taking 10+ seconds.

eerison commented 2 months ago

Hello @Seldaek

as it was found on version 2.x, couldn't it be released on version 2.x? this way we could still have this issue solved on php 8.0 and 8.2, and make a smooth php upgrade :)

Edit: I just saw there is a PR for 2.x :)

Seldaek commented 2 months ago

There is https://github.com/Seldaek/monolog/pull/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.

ChipZ91 commented 2 months ago

Unfortunately we have the same error despite the change.

ivanmartinvalle commented 2 months ago

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();
    }
}
ChipZ91 commented 2 months ago

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.

Seldaek commented 2 weeks ago

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.

Seldaek commented 2 weeks ago

See https://github.com/Seldaek/monolog/pull/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.

Seldaek commented 2 weeks ago

Closing in favor of https://github.com/Seldaek/monolog/pull/1882 which is now merged in 2.x-dev and 3.x-dev