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

Prevent infinite loop on writing to broken pipes #70

Closed
wants to merge 1 commit into from
Closed

Prevent infinite loop on writing to broken pipes #70

wants to merge 1 commit into from

Conversation

dangoodman
Copy link

@dangoodman dangoodman commented Jan 23, 2020

The assertion "Trying to write on a previously fclose()'d resource. Do NOT manually fclose() resources the loop still has a reference to." happens to me even if I have not a single fclose(), stream_XXX() or socket_XXX() call in my code.

My client and server thin wrappers around Server::listen() and connect() are running with Docker containers on Windows. So it might be related to docker networking internals, breaking sockets under some circumstances.

It happens occassionaly. I spent a whole day to find steps to reproduce it with no luck.

Anyway, since the case does really happen sometimes in runtime, I think it would be better to handle it and prevent the infinite loop.

@trowski
Copy link
Member

trowski commented Jan 23, 2020

I assume you're using PHP 7.4.0 or 7.4.1? If so, this is a known bug that was fixed in 7.4.2.

@dangoodman
Copy link
Author

dangoodman commented Jan 23, 2020

Right, PHP 7.4.1. Still get the error with PHP 7.4.2 though.

Found a way to reproduce the issue:
Test server
Test client
Example output of test-client.php with PHP 7.1-7.4.2
Example output of test-server.php with PHP 7.4.2 and small chunk size
Example output of test-server.php with PHP 7.1-7.3 and small chunk size
Example output of test-server.php with PHP 7.1-7.4.2 and regular chunk size

The issue is seemingly related to the read chunk size. When it's set to a small value, e.g. 3 bytes, write to the socket fails.

Also, a similar issue happens to PHP 7.1-7.3. It throws SocketException with message 'Failed to write to stream after multiple attempts; fwrite(): send of XX bytes failed with errno=32 Broken pipe'. Although it seems to be a legit condition, it disappears once we get back to the regular read chunk size, just as with 'Trying to write on a previously fclose()'d resource'.

Not sure what's causing it and whether there is an implied limit on socket read chunk size.

@trowski
Copy link
Member

trowski commented Jan 23, 2020

Can you please try with the fix-eagain branch and see if you still get assertion failures on 7.4.2 with small chunk sizes?

@dangoodman
Copy link
Author

I tried it with my commit. It doesn't seem to be different to the fix-eagain, if keep PHP 7.4.2 in mind. Am I missing something?

@trowski
Copy link
Member

trowski commented Jan 23, 2020

Yes, it was nearly the same as your commit. I pushed another commit to the branch, since it appears EPIPE now returns false in 7.4.2. Not sure if that's related to your issue, so could you now try the fix-eagain branch?

@dangoodman
Copy link
Author

With PHP 7.4.2, fix-eagain works now as with PHP 7.1-7.3, i.e. emits SocketException 'fwrite(): send of 23 bytes failed with errno=32 Broken pipe'.

@kelunik
Copy link
Member

kelunik commented Jan 29, 2020

Closing in favor of #71. Thanks!

@kelunik kelunik closed this Jan 29, 2020
@dangoodman
Copy link
Author

Thanks.

How about an issue with with PHP 7.1-7.3?

It throws SocketException with message 'Failed to write to stream after multiple attempts; fwrite(): send of XX bytes failed with errno=32 Broken pipe'. Although it seems to be a legit condition, it disappears once we get back to the regular read chunk size, just as with 'Trying to write on a previously fclose()'d resource'.

The issue can be reproduced and fixed in the same way, mentioned above.

Should I post another issue?

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

Successfully merging this pull request may close these issues.

3 participants