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

Do not die after failing to load app.psgi #117

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kappa

kappa commented Sep 27, 2015

This stops the error flood in logs when app.psgi is broken
due to endless forking of immediately dying children.

Fixes #94 and #106.

Do not die after failing to load app.psgi
This stops the error flood in logs when app.psgi is broken
due to endless forking of immediately dying children.
@ap

This comment has been minimized.

Show comment
Hide comment
@ap

ap Oct 4, 2015

That’s not a good idea for detecting whether an exception has been thrown, especially on older perls. It should be written eval { $self->{app} = $self->{options}->{psgi_app_builder}->(); 1 } or do { ... }, so that it doesn’t depend on $@ to check whether an exception has been thrown. (Once you know it has, then looking at the value of $@ is reliable enough.))

ap commented on lib/Starman/Server.pm in 6e63c3d Oct 4, 2015

That’s not a good idea for detecting whether an exception has been thrown, especially on older perls. It should be written eval { $self->{app} = $self->{options}->{psgi_app_builder}->(); 1 } or do { ... }, so that it doesn’t depend on $@ to check whether an exception has been thrown. (Once you know it has, then looking at the value of $@ is reliable enough.))

This comment has been minimized.

Show comment
Hide comment
@kappa

kappa Oct 4, 2015

Owner

I was under impression that this is important if and only if we have some code between the check on $@ and the eval. I will fix it.

Owner

kappa replied Oct 4, 2015

I was under impression that this is important if and only if we have some code between the check on $@ and the eval. I will fix it.

This comment has been minimized.

Show comment
Hide comment
@ap

ap Oct 4, 2015

It’s more complicated than that. The BACKGROUND section in Try::Tiny’s docs lists the major edge cases, at least as they once were. If my quick recheck is right, the improvements to $@ are not mentioned there – perl 5.14 in particular fixed the most important problem listed (exceptions in destructors). But it remains tricky to use $@ right, and using the return value of eval instead of checking $@ is the most basic thing you can do to detect exceptions reliably. I admit I’m not certain how necessary it is on recent perls, iff you make sure that all the conditions of its use line up just right… but it’s mandatory if you want to support older perls anyway.

ap replied Oct 4, 2015

It’s more complicated than that. The BACKGROUND section in Try::Tiny’s docs lists the major edge cases, at least as they once were. If my quick recheck is right, the improvements to $@ are not mentioned there – perl 5.14 in particular fixed the most important problem listed (exceptions in destructors). But it remains tricky to use $@ right, and using the return value of eval instead of checking $@ is the most basic thing you can do to detect exceptions reliably. I admit I’m not certain how necessary it is on recent perls, iff you make sure that all the conditions of its use line up just right… but it’s mandatory if you want to support older perls anyway.

This comment has been minimized.

Show comment
Hide comment
@kappa

kappa Oct 6, 2015

Owner

Thanks for the explanation. I added the fix to the pull request.

Owner

kappa replied Oct 6, 2015

Thanks for the explanation. I added the fix to the pull request.

Catch more properly
As suggested by Aristotle Pagaltzis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment