fix: Wrap weasyprint to catch exceptions#6728
Conversation
|
Per real-time discussion - try using mock in the test. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6728 +/- ##
==========================================
+ Coverage 88.78% 88.80% +0.01%
==========================================
Files 285 285
Lines 40324 40286 -38
==========================================
- Hits 35800 35774 -26
+ Misses 4524 4512 -12 ☔ View full report in Codecov by Sentry. |
|
|
||
| pdf = doc.pdfized() | ||
| try: | ||
| pdf = doc.pdfized() |
There was a problem hiding this comment.
when this fails, it's after a fairly long timeout - I wonder if two timeouts will take us past the cloudflare timeout window?
There was a problem hiding this comment.
Maybe add a log to indicate how often a retry occurs? (Assuming that general failures are also logged, this would let us evaluate whether it's worth trying a second time)
Longer term, this would be a candidate to make into an async response and defer to celery.
| pdf = doc.pdfized() | ||
| try: | ||
| pdf = doc.pdfized() | ||
| except URLFetchingError: |
There was a problem hiding this comment.
This is what we've seen it throw, but I don't want it to emit some other exception and float it all the way back to a 5xx crash either. I think a general catch here, logging the error, and returning a "something went wrong, and someone will be looking into it. please try again later" message would be better?
There was a problem hiding this comment.
I'd also suggest not bubbling weasyprint dependencies up into the view. The pdfize() method should really handle those and raise its own error class (or classes if it needs to distinguish, say, temporary from permanent problems or other classifications).
| except URLFetchingError as exception: | ||
| return render(request, "doc/weasyprint_failed.html", | ||
| dict(error=f'{type(exception).__name__}: {exception}'), | ||
| status=504) |
There was a problem hiding this comment.
While 504 here is one technically correct to represent the situation , the key point of the issue is to not return any 5xx.
I would suggest a 200.
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Small (but I think important) nit.
| return render(request, "doc/weasyprint_failed.html", | ||
| dict(error=f'{type(exception).__name__}: {exception}'), | ||
| status=504) | ||
| except: |
There was a problem hiding this comment.
Should probably be except Exception - a bare except catches low-level non-Exception interruptions that shouldn't normally be caught without re-raising.
3 participants
Fixes #6324