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

Reduce scope of middleware exception handling #476

Conversation

steventux
Copy link
Contributor

Limit the scope of exception handling to the PDF generation logic in the middleware.

The call to @app.call(env) should be able to raise exceptions as these may be legitimate errors from applications or other middlewares.

We noticed that in development mode, our application errors were being obfuscated and caught by the rescue statement for the entire middleware, this appears to be because we also trap errors coming via @app.call(env).

cc @serene - I believe this relates to the review conversation we had here #469 (review) please let me know if you need more information.

@steventux steventux force-pushed the reduce-scope-of-middleware-exception-handling branch 3 times, most recently from fd5bc9f to f603240 Compare July 8, 2020 08:31
@serene
Copy link
Contributor

serene commented Jul 8, 2020

@steventux Test please :)

Limits the handling of exceptions to those occurring in the PDF generation logic,
handling errors from @app.call(env) has the side-effect of burying errors which may
be raised legitimately by other middlewares.
@steventux steventux force-pushed the reduce-scope-of-middleware-exception-handling branch from f603240 to d60904e Compare July 8, 2020 14:49
@steventux
Copy link
Contributor Author

@serene 👍 I've added a test here now d60904e#diff-115284d51cd4c5774a89ff4272da74a4R410-R418

@serene serene added the ready label Jul 8, 2020
@serene serene merged commit 17f4efe into pdfkit:master Jul 8, 2020
@fmluizao
Copy link

fmluizao commented Jul 9, 2020

It would be possible to release a new version with this?

0.8.4.3.1 is catching application exceptions...

@steventux
Copy link
Contributor Author

Hi @serene any news on the next release of pdfkit? Is there likely to be another patch to introduce d60904e?

@serene serene removed the ready label Aug 16, 2020
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

3 participants