fix(listeners): isolate listener exceptions from message dispatch loop - #1953
Open
alaahong wants to merge 1 commit into
Open
fix(listeners): isolate listener exceptions from message dispatch loop#1953alaahong wants to merge 1 commit into
alaahong wants to merge 1 commit into
Conversation
A listener throwing on the message dispatch thread could propagate up through Connection.dispatch and break unrelated API calls that happened to pump messages while the listener ran (e.g. download.path() aborted by a TargetClosedError thrown from a waitForLoadState listener). This mirrors the JS client where async listener rejections surface as unhandled rejections on stderr without interrupting the caller. Fixes: microsoft#1952
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ListenerCollection.notify()so a listener throwing on the message-dispatch thread can no longer propagate up throughConnection.dispatchand break unrelated API calls (e.g.download.path(),page.close()) that happen to pump messages while the listener runs.Root Cause
Fixes #1952
ListenerCollection.notify()called each listener without any exception isolation. When a listener threw (for example, aTargetClosedErrorfrom awaitForLoadStatecall made after the page was closed), the exception propagated synchronously up through:ListenerCollection.notify → BrowserContextImpl.handleEvent → Connection.dispatch → Connection.processOneMessage → ChannelOwner.runUntil ← outer API call's message loop → ArtifactImpl.pathAfterFinished → DownloadImpl.path
This broke the outer
download.path()call even though the listener failure was unrelated to it. The issue was introduced in 1.59.0 and reproduced withdownload.createReadStream()/download.path()afterpage.waitForDownload().Fix
Wrap each listener invocation in
ListenerCollection.notify()withtry/catch:Why this aligns with the JS client
The JS client's
EventEmitter._callHandlerrelies on async isolation — listeners return Promises, andpromise.catch(e => { throw e; })turns failures into unhandled rejections that Node prints to stderr by default, without interrupting the caller's Promise chain.The Java client is synchronous: listeners are
Consumer<T>andpage.waitForLoadState()throwsTargetClosedErrorsynchronously. There is no natural async isolation, so thetry/catchhere explicitly mirrors the JSpromise.catchbehavior:Reflect.apply(handler, this, args)((Consumer<T>) listener).accept(param)promise.catch(e => { throw e; })try { ... } catch (RuntimeException e) { ... }runUntil)System.err.printlnalways prints to stderrLoggingSupport.logApiIfEnabledadds a structuredpw:apilog lineforloop continues)forloop continues)removeAllListeners(type, { behavior: 'ignoreErrors' })sets_rejectionHandler = () => {}Tests
TestListenerCollection(new, 4 tests): pure unit tests verifying a throwing listener does not break the dispatch loop, remaining listeners are still called, and the exception is printed to stderr for observability.TestDownload#shouldNotBreakMessageDispatchWhenListenerThrows(new): integration test that registers a throwingonCloselistener and assertspage.close()(which pumps messages and triggers the listener) does not throw.Verified by temporarily reverting only the
ListenerCollection.javafix and re-running the integration test — it failed withRuntimeException: listener threw during message dispatch(reproducing #1952), then passed again after re-applying the fix.