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

ExtEventLoop: No longer suppress all errors #65

Merged
merged 1 commit into from Feb 15, 2017

Conversation

Projects
None yet
5 participants
@mamciek

mamciek commented Jan 9, 2017

No description provided.

@@ -178,8 +178,7 @@ public function tick()
$this->futureTickQueue->tick();
// @-suppression: https://github.com/reactphp/react/pull/234#discussion-diff-7759616R226
@$this->eventBase->loop(EventBase::LOOP_ONCE | EventBase::LOOP_NONBLOCK);
$this->eventBase->loop(EventBase::LOOP_ONCE | EventBase::LOOP_NONBLOCK);

This comment has been minimized.

@clue

clue Jan 9, 2017

Member

How can we verify the above comment is no longer relevant?

Can you add some tests to verify this?

This comment has been minimized.

@mamciek

mamciek Jan 9, 2017

I do not know if the above comment is relevant or not. I do know that this @-suppression supress ALL php errors that is ever generated by PHP (inside event handler) which is very bad

This comment has been minimized.

@clue

clue Jan 9, 2017

Member

I understand, but if fix this now we should make sure to avoid any regressions in the future.

Can you add some tests to verify this?

This comment has been minimized.

@jsor

jsor Feb 3, 2017

Member

I've did some tests today but i can't reproduce this. The comment in reactphp/react#234 (diff) is also not clear in what exactly produces these warnings. Maybe @nrk or @jmalloc can shed some light on this.

I agree that the @ is really bad here because it also supresses any warnings and errors from the callbacks.

This comment has been minimized.

@jmalloc

jmalloc Feb 6, 2017

Contributor

IIRC, the initial problem was that EventBase::loop() emits an (additional?) warning after an error in a user-defined callback. So, perhaps calling trigger_error() in a timer function, or something like that would reproduce it. I can't recall if I witnessed this myself, but I kept the @-suppression at @nrk's request.

I can see where this warning might be an issue, especially when using a strict error handler, but it's arguably the responsibility of the user to handle/suppress any such errors in callbacks. So as long as we're not producing them in the loop implementation ourselves it's probably OK to remove the @.

This comment has been minimized.

@clue

clue Feb 7, 2017

Member

In the meanwhile I can open an issue on pecl-event to let the author know about this.

Yes, pushing this upstream is where this should be addressed in the first place! 👍

I'd say it's better to have additional warnings than suppressing everything.
[…]
3. Remove the @-suppression and document the change and it's impact in the changelog

Absolutely agree here! 👍

Given this only triggers when an uncaught warning or exception is passed through, I expect this to have very little impact in actual consumer code. After all, this is something that should never happen in the first place. Also, this only triggers an additional warning, so I agree this should be safe to add regardless 👍

This means that even if upstream takes longer to fix this and this be inconsistent across different upstream versions, I think it makes perfect sense to get this in now already 👍

This comment has been minimized.

@nrk

nrk Feb 7, 2017

Member

Given this only triggers when an uncaught warning or exception is passed through, I expect this to have very little impact in actual consumer code.

@clue the additional warning is emitted even when the exception is caught, see the example in my previous comment.

This comment has been minimized.

@jsor

jsor Feb 8, 2017

Member

@jsor alright will do tomorrow first thing in the morning!

@nrk Are you still planning to file an issue in the pecl-event repo? Otherwise i can do it.

@clue the additional warning is emitted even when the exception is caught, see the example in my previous comment.

Yes, but only if the exception bubbles up to the $this->eventBase->loop() call. This usually results in an uncaught exception anyway since in almost all cases $loop->run() is called at the end of the script.

This comment has been minimized.

@nrk

nrk Feb 8, 2017

Member

@jsor sure that's still my plan, actually I was modifying the extension myself to test this change and eventually have a PR ready if the author agrees, just ended up having unexpected stuff going on at work so couldn't do it yet. As for uncaught exceptions: yes and I realized later that I misinterpreted what @clue meant with passed through.

This comment has been minimized.

@jsor

jsor Feb 8, 2017

Member

sure that's still my plan, actually I was modifying the extension myself to test this change and eventually have a PR ready if the author agrees, just ended up having unexpected stuff going on at work so couldn't do it yet.

No hurries, very appreciated 👍

@clue clue added bug easy pick labels Jan 9, 2017

@mamciek

This comment has been minimized.

mamciek commented Feb 7, 2017

The problem is half solved now :) One of the @ suppression was removed with 8fa0407

@jsor

jsor approved these changes Feb 8, 2017

👍 for getting this in, i'll do a followup PR with a changelog entry

@clue clue changed the title from Remove @-suppression from event loop to ExtEventLoop: No longer suppress all errors Feb 8, 2017

@clue

clue approved these changes Feb 8, 2017

Also 👍 from my end.

See also above for some very good discussion about testing this: #65 (comment)
Short version, we can not reliably test the @ is needed in the first place, but we have a good understanding of the problem at hand, so we can omit a test here IMHO.

@clue clue added this to the v0.4.3 milestone Feb 8, 2017

@clue

This comment has been minimized.

Member

clue commented Feb 8, 2017

@mamciek May I ask you to rebase this on the new "0.4" branch so we get this in for the upcoming v0.4.3 release? :shipit:

@mamciek mamciek changed the base branch from master to 0.4 Feb 15, 2017

@mamciek

This comment has been minimized.

mamciek commented Feb 15, 2017

Sure. Done.

@jsor jsor merged commit f5a8549 into reactphp:0.4 Feb 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsor

This comment has been minimized.

Member

jsor commented Feb 15, 2017

Thanks! 👍

@jsor jsor referenced this pull request Feb 15, 2017

Merged

Update master from 0.4 #81

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