Skip to content

[ZEPPELIN-6638] Time out the React remote entry load - #5409

Open
kimyenac wants to merge 1 commit into
apache:masterfrom
kimyenac:ZEPPELIN-6638
Open

[ZEPPELIN-6638] Time out the React remote entry load#5409
kimyenac wants to merge 1 commit into
apache:masterfrom
kimyenac:ZEPPELIN-6638

Conversation

@kimyenac

Copy link
Copy Markdown
Contributor

What is this PR for?

ReactRemoteLoaderService settles loadContainer() only from the script tag's onload and onerror. Neither fires while a request is merely pending, and there is no timer. A remoteEntry.js request that the server accepts and never answers therefore leaves the promise pending for as long as the browser keeps the connection open, which is minutes.

onError is never called, so the hosts that depend on it never fall back. The paragraph footer keeps an empty mount div instead of restoring zeppelin-notebook-paragraph-footer, and the published paragraph renders nothing. Both look like a slow page rather than a failed load.

This bounds the script load with environment.reactRemoteLoadTimeoutMs (10 s, or 0 to disable) and reuses the existing fail() path on expiry, which removes the tag and leaves the caches drained so a later mount can retry.

The chunks that container.get() pulls are left alone. They are fetched by the remote's own webpack runtime, which already bounds them with output.chunkLoadTimeout (120 s by default). A second, shorter timer over that path would cut off a multi-megabyte chunk on a slow connection, which is a worse failure than the one being fixed.

What type of PR is it?

Bug Fix

Todos

None

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-6638

How should this be tested?

  • Two new Playwright cases in e2e/tests/notebook/paragraph/react-footer.spec.ts: one holds remoteEntry.js open without ever answering and asserts the Angular footer comes back, one answers after a delay well inside the budget and asserts the React footer still renders. The first fails on master and passes here; verified by setting the budget to 0, which reproduces the current behaviour and makes it fail.
  • The existing abort and delay cases in the same suite, and e2e/tests/notebook/published/published-paragraph.spec.ts, for regressions. Chromium, all green.
  • A production build (npm run build).
  • A unit spec for the service would be the better home for the timer, but the Angular shell has no test harness on master yet (ZEPPELIN-6566, ZEPPELIN-6567, ZEPPELIN-6637) and this file carries an @Injectable decorator, which the current setup cannot compile. Left to a follow-up once that lands.

Screenshots (if appropriate)

No

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes, the loader section of projects/zeppelin-react/README.md is updated in this PR

ReactRemoteLoaderService settles loadContainer() only from the script
tag's onload and onerror. Neither fires while a request is merely
pending, so a remoteEntry.js request that is accepted and never answered
leaves the promise pending for as long as the browser holds the
connection. onError is never called, and the hosts that depend on it
never fall back: the paragraph footer keeps an empty mount div instead
of restoring the Angular footer, and the published paragraph renders
nothing.

Bound the script load with environment.reactRemoteLoadTimeoutMs and
reuse the existing fail() path on expiry, which removes the tag and
leaves the caches drained so a later mount can retry. Set it to 0 to
disable the timer.

The chunks that container.get() pulls are left alone. They are fetched
by the remote's own webpack runtime, which already bounds them with
output.chunkLoadTimeout, and a second shorter timer would cut off a
multi-megabyte chunk on a slow connection.

Covered by two Playwright cases: one that holds remoteEntry.js open
without ever answering and asserts the Angular footer comes back, and
one with a delayed but successful response that asserts the React
footer still renders.

@voidmatcha voidmatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants