Propagate network errors through promise

[alt-]

I'm trying to catch transient network errors on range requests in PDFDocmentProxy::getPage, but they don't appear to propagate through the promise.

I tracked it down to ChunkedStreamManager::sendRequest not attaching an error handler to the NetworkManager::requestRange call.

ChunkedStreamManager::onError already exists, but doesn't appear to be used anywhere. Attaching it to the call triggers a Uncaught (in promise) 0 on a network error, so there appears to be more plumbing necessary to propagate the error through the promise.

I'd be happy to create a pull request if this seems like a logical path to follow, but I'm wondering if there are API changes required and would like a second opinion before starting.

[yurydelendik]

I'd be happy to create a pull request if this seems like a logical path to follow, but I'm wondering if there are API changes required and would like a second opinion before starting.

I don't think API change will be required, however there might be some extensive changes in the core files. Feel free to submit work-in-progress code and iterate in the PR, so you can receive earlier feedback on your effort.

Can you also provide STR e.g. by modifying test server, so it will be possible to simulate an error you are referring to?

[alt-]

I have a working version in my branch, but am having trouble getting Firefox to pass the new unit test PDFDocument::reports failure to get page as it fetches the whole file faster than I can cut the connection.

I did a minor API change to PDFManager::requestLoadedStream. It now returns a promise that can fail due to a network error and not the same promise as PDFManager::onLoadedStream, which never fails, as it may fire later after the network recovers and the missing chunks are retried.

Various tweaks were also added to the promise caching to allow for retries after failure.

The test required a separate web server instance so I could simulate a network error (ie. cut the connection) without breaking other tests, so now exposes a way for the tests to instantiate and stop web servers.

I'd very much appreciate any comments. I'll attempt to fix the unit test on Firefox, but it may take a while.

[timvandermeij]

@alt- At first glance I notice that you do not cache loop variables. Could you change, for instance, for (i = 0; i < requestIds.length; ++i) { to for (i = 0, len = requestIds.length; i < len; ++i) { such that requestIds.length does not need to be evaluated for each iteration? Other than that code style wise this is looking great and I'm really happy to see errors being handled in a much better way! @yurydelendik can probably help you with the tests. Thanks!

[alt-]

I managed to get the unit test to work on Firefox by rate limiting the web server.

That should hopefully be it for this issue.
Please take a look at PR #6149 and let me know if there's anything else you want me to tweak.

[Snuffleupagus]

I believe that this issue is now fixed, thanks to a fair number of PRs that have re-factored and improved the code over the years (with the recent #13937 and #13945 taking care of the final parts).