From f1487714b4ec0accb3e88e375849769ca32e6356 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 1 Aug 2022 12:16:29 +0100 Subject: [PATCH] Merge pull request #9058 from overleaf/jpa-error-handling-2nd-page [web] improve error handling for pdf-caching GitOrigin-RevId: ddd65f2813fb08eb4493967071ee62d9742930cc --- .../pdf-preview/components/pdf-js-viewer.js | 20 ++++++++++--- .../pdf-preview/util/pdf-caching-transport.js | 29 ++++++++++++------- .../pdf-preview/util/pdf-js-wrapper.js | 9 ++++-- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js index d7b99cf974..f72333c756 100644 --- a/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js +++ b/services/web/frontend/js/features/pdf-preview/components/pdf-js-viewer.js @@ -127,11 +127,23 @@ function PdfJsViewer({ url, pdfFile }) { setError(undefined) setStartFetch(performance.now()) - pdfJsWrapper.loadDocument(url, pdfFile).catch(error => { - console.error(error) + const abortController = new AbortController() + const handleFetchError = () => { + if (abortController.signal.aborted) return + // The error is already logged at the call-site with additional context. setError('rendering-error') - }) - return () => pdfJsWrapper.abortDocumentLoading() + } + pdfJsWrapper + .loadDocument({ url, pdfFile, abortController, handleFetchError }) + .catch(error => { + if (abortController.signal.aborted) return + console.error(error) + setError('rendering-error') + }) + return () => { + abortController.abort() + pdfJsWrapper.abortDocumentLoading() + } } }, [pdfJsWrapper, url, pdfFile, setError, setStartFetch]) diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js index f347bde51d..4507583c02 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js @@ -9,10 +9,10 @@ export function generatePdfCachingTransportFactory(PDFJS) { if (!enablePdfCaching && !trackPdfDownloadEnabled) { return () => null } - let failedOnce = false const cached = new Set() const metrics = Object.assign(getPdfCachingMetrics(), { failedCount: 0, + failedOnce: false, tooLargeOverheadCount: 0, tooManyRequestsCount: 0, cachedCount: 0, @@ -27,12 +27,12 @@ export function generatePdfCachingTransportFactory(PDFJS) { new URLSearchParams(window.location.search).get('verify_chunks') === 'true' class PDFDataRangeTransport extends PDFJS.PDFDataRangeTransport { - constructor(url, pdfFile, reject) { + constructor({ url, pdfFile, abortController, handleFetchError }) { super(pdfFile.size, new Uint8Array()) this.url = url this.pdfFile = pdfFile - this.reject = reject - this.abortController = new AbortController() + this.handleFetchError = handleFetchError + this.abortController = abortController } abort() { @@ -59,17 +59,18 @@ export function generatePdfCachingTransportFactory(PDFJS) { abortSignal, }) .catch(err => { + if (abortSignal.aborted) return if ( err.message === 'non successful response status: 404' && OError.getFullInfo(err).url === this.url ) { // Do not consider a 404 on the main pdf url as pdf caching failure. // Still, bail out during the initial launch phase. - failedOnce = true + metrics.failedOnce = true throw new PDFJS.MissingPDFException() } metrics.failedCount++ - failedOnce = true + metrics.failedOnce = true if (!enablePdfCaching) { throw err // This was a fallback request already. Do not retry. } @@ -79,25 +80,33 @@ export function generatePdfCachingTransportFactory(PDFJS) { return fallbackRequest({ url: this.url, start, end, abortSignal }) }) .then(blob => { + if (abortSignal.aborted) return this.onDataRange(start, blob) }) .catch(err => { + if (abortSignal.aborted) return err = OError.tag(err, 'fatal pdf download error', errorInfo) console.error(err) if (!(err instanceof PDFJS.MissingPDFException)) { captureException(err, { tags: { fromPdfCaching: true } }) } - this.reject(err) + // Signal error for (subsequent) page load. + this.handleFetchError(err) }) } } - return function (url, pdfFile, reject) { - if (failedOnce) { + return function ({ url, pdfFile, abortController, handleFetchError }) { + if (metrics.failedOnce) { // Disable pdf caching once any fetch request failed. // Be trigger-happy here until we reached a stable state of the feature. return null } - return new PDFDataRangeTransport(url, pdfFile, reject) + return new PDFDataRangeTransport({ + url, + pdfFile, + abortController, + handleFetchError, + }) } } diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js index 99fc238248..2648a8fbcb 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-js-wrapper.js @@ -64,7 +64,7 @@ export default class PDFJSWrapper { } // load a document from a URL - loadDocument(url, pdfFile) { + loadDocument({ url, pdfFile, abortController, handleFetchError }) { // cancel any previous loading task if (this.loadDocumentTask) { this.loadDocumentTask.destroy() @@ -72,7 +72,12 @@ export default class PDFJSWrapper { } return new Promise((resolve, reject) => { - const rangeTransport = this.genPdfCachingTransport(url, pdfFile, reject) + const rangeTransport = this.genPdfCachingTransport({ + url, + pdfFile, + abortController, + handleFetchError, + }) let rangeChunkSize = DEFAULT_RANGE_CHUNK_SIZE if (rangeTransport && pdfFile.size < 2 * DEFAULT_RANGE_CHUNK_SIZE) { // pdf.js disables the "bulk" download optimization when providing a