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

added @ before stream_socket_accept in src/Server.php ... #23

Merged
merged 1 commit into from
Jan 19, 2016

Conversation

krageon
Copy link

@krageon krageon commented Sep 16, 2015

… , preventing the server from crashing right before being able to handle the same error

This fixes the following issue:
#22

… server from crashing right before being able to handle the same error
@clue
Copy link
Member

clue commented Sep 16, 2015

The changes LGTM 👍

Can you give some steps to reproduce this so we can add this to the test suite? Thanks!

@WyriHaximus
Copy link
Member

+1 on @clue comment, the changes LGTM but steps to reproduce are very welcome 👍

@krageon
Copy link
Author

krageon commented Sep 17, 2015

I discovered the problem because an external monitoring tool (HAProxy) opened a TCP socket and closed it immediately after it was opened, causing the exception. That is the reason I do not have a self-contained problem case.

Edit:
Apologies, I think I didn't read properly this morning.

The way to reproduce this is:

  • Start server on port
  • Open socket to port from client
  • Close socket immediately after opening

This will cause an E_WARNING to pop up ( http://php.net/manual/en/stream.errors.php )

@cboden
Copy link
Member

cboden commented Sep 17, 2015

I'm very weary about using @; it's detrimental to performance and this will happen on every new connection. A less elegant, yet more performant method of achieving this is to set and restore the global error handler where this type of issue can arise.

@krageon
Copy link
Author

krageon commented Sep 18, 2015

It doesn't seem to be slower than setting and restoring the global error handler. Test: https://3v4l.org/Pscrm

I don't really have any qualms with changing it either way, though. Thoughts?

@dennisdegreef
Copy link

I'm surprised to see those statistics. As far as I know, the 'silence'-operator was indeed slow as well.

@WyriHaximus
Copy link
Member

I'm also surprised by those numbers. Reading @derickr's article on the @ operator: http://derickrethans.nl/five-reasons-why-the-shutop-operator-should-be-avoided.html and if I'm interpreting reason 3 part 2 correctly it means that the code is slow because all the error message generating and formatting happens only when there is an error. So that is a good thing but reason 3 part 1 states that every time the @ operator is used the INI directives are used to suppress it. This happens every time stream_socket_accept is called.

I'm still in favor of using the @ operator to fix the rather nasty #22 👍

@derickr
Copy link

derickr commented Oct 8, 2015

I agree @WyriHaximus

@clue
Copy link
Member

clue commented Nov 21, 2015

Let's move forward and get this in? :shipit:

If anybody has some numbers to back up any performance regressions or happens to write a (better?) patch, please file a new PR and I'm happy to consider this again!

@WyriHaximus
Copy link
Member

Agreed :shipit:

@cboden cboden added this to the v0.4.3 milestone Jan 19, 2016
cboden added a commit that referenced this pull request Jan 19, 2016
added @ before stream_socket_accept in src/Server.php ...
@cboden cboden merged commit b4baa1e into reactphp:master Jan 19, 2016
clue pushed a commit to clue-labs/socket that referenced this pull request Mar 29, 2017
Added peer_name to ssl context options
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.

None yet

6 participants