Prevent Uncaught (in promise) AbortException when running the unit-tests
#12144
Conversation
|
/botio unittest |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/86238e08d994ec6/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/431e719b47e6bda/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/86238e08d994ec6/output.txt Total script time: 3.74 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/431e719b47e6bda/output.txt Total script time: 4.92 mins
|
| sink.ready.catch(readyReason => { | ||
| if (this.destroyed) { | ||
| return; // Ignore any pending requests if the worker was terminated. | ||
| } | ||
| throw readyReason; | ||
| }); |
There was a problem hiding this comment.
Compared to the fullReader case above, I wasn't actually able to trigger this code-path in any of the unit-tests. However, given the (fairly) small amount of code, the fact that this is consistent with the above, and to prevent any future issues I figured that this cannot hurt.
Given the (relatively) small size of a range request, as compared to the full request, triggering this is probably a lot more difficult and timing-dependent. E.g. it may still happen for a slow connection and/or a large rangeChunkSize option.
| return capability.promise.catch(reason => { | ||
| if (this.aborted) { | ||
| return; // Ignoring any pending requests after abort. | ||
| } | ||
| throw reason; | ||
| }); |
There was a problem hiding this comment.
These changes weren't necessary to get the unit-tests to run as-is with an updated Jasmine version, however with disableStream = true set they still failed with Uncaught (in promise) AbortException which thus necessitated these changes.
Given that we're already ignoring read data, in sendRequest, this is to some extent thus "consistent" behaviour.
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/8176f2c172adc23/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/277d512d129a4a2/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/8176f2c172adc23/output.txt Total script time: 26.73 mins
Image differences available at: http://54.67.70.0:8877/8176f2c172adc23/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/277d512d129a4a2/output.txt Total script time: 29.54 mins
Image differences available at: http://54.215.176.217:8877/277d512d129a4a2/reftest-analyzer.html#web=eq.log |
|
Unfortunately there's apparently one remaining unit-test failure, which is limited to Google Chrome on the Linux bot: Perhaps unsurprisingly, I cannot reproduce this locally in Chrome on Windows since [1] Unfortunately, even the patch as-is took me hours of debugging/testing, spread out over a couple of days, to arrive at. |
I would say that these changes make sense if the Jasmine update itself is left out of this PR until the last failure is fixed. |
…tests These errors can/will occur if data is still loading when the document is destroyed, which is the case in the API unit-tests that load the `tracemonkey.pdf` file. While this patch prevents these kind of problems, and thus allows us to update Jasmine again, I cannot help but thinking that it's slightly "hacky". Basically, we'll simply catch and ignore (some) rejected promises once the document is destroyed and/or its data loading is aborted. However, I don't *think* that these changes should cause issues in general, since we don't really care about errors once document destruction has started (note e.g. the fair number of `catch` handlers ignoring `AbortException`s already).
Snuffleupagus
force-pushed
the
uncaught-promise-AbortException
branch
from
0ce25d9 to
6d192f9
Compare
Done; although I do find it to be quite unfortunate having to do so, since I worry that we'll "never" actually get the final test-failure fixed and thus be stuck at an old Jasmine-version indefinitely. (See e.g. issue #11996 which tracks another, fairly old, dependency problem that may sooner of later become a real problem.) /botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/93ce393d3fb967f/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/8ab0e4e1a5c7388/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/8ab0e4e1a5c7388/output.txt Total script time: 3.90 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/93ce393d3fb967f/output.txt Total script time: 4.96 mins
|
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/a07cc93e5c5224f/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/46c1c9184bc186a/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/46c1c9184bc186a/output.txt Total script time: 26.85 mins
Image differences available at: http://54.67.70.0:8877/46c1c9184bc186a/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/a07cc93e5c5224f/output.txt Total script time: 29.68 mins
Image differences available at: http://54.215.176.217:8877/a07cc93e5c5224f/reftest-analyzer.html#web=eq.log |
|
Looks good to me; another step closer to updating Jasmine in the end. |
These errors can/will occur if data is still loading when the document is destroyed, which is the case in the API unit-tests that load the
tracemonkey.pdffile.While this patch prevents these kind of problems, and thus allows us to update Jasmine again, I cannot help but thinking that it's slightly "hacky". Basically, we'll simply catch and ignore (some) rejected promises once the document is destroyed and/or its data loading is aborted. However, I don't think that these changes should cause issues in general, since we don't really care about errors once document destruction has started (note e.g. the fair number of
catchhandlers ignoringAbortExceptions already).