fix: Defer page object cleanup to make it accessible in error handlers#1814
fix: Defer page object cleanup to make it accessible in error handlers#1814
Conversation
janbuchar
commented
Mar 27, 2026
janbuchar
commented
- closes PlaywrightCrawler error_handler cannot access Page object #1482
janbuchar
added
the
t-tooling
github-actions
bot
added
the
tested
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1814 +/- ##
=======================================
Coverage 92.25% 92.26%
=======================================
Files 157 157
Lines 10847 10895 +48
=======================================
+ Hits 10007 10052 +45
- Misses 840 843 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
Just a few comments/questions, otherwise LGTM
| for cleanup in deferred_cleanup: | ||
| try: | ||
| await cleanup() | ||
| except Exception: # noqa: PERF203 |
There was a problem hiding this comment.
Can't we put the entire loop into the try-except block as the lint rule suggests?
There was a problem hiding this comment.
That would mean that if one cleanup hook fails, all those that follow it will, too. I wouldn't worry about performance too much here, there won't be more than one for 99% of users.
| with contextlib.suppress(Exception): | ||
| await context.page.__aexit__(None, None, None) |
There was a problem hiding this comment.
Should we really suppress all exceptions completely? Maybe we could at least log them? In the BasicCrawler cleanup loop, we're already catching and logging exceptions, so we could even let them propagate to that level.
Just an idea, maybe there is a good reason to completely suppress all exceptions on this level.
| log: logging.Logger | ||
| """Logger instance.""" | ||
|
|
||
| register_deferred_cleanup: Callable[[Callable[[], Coroutine[None, None, None]]], None] |
There was a problem hiding this comment.
The type is huge, maybe we could utilize a type alias DeferredCleanupCallback?
3 participants
