chadek commented on PR #44470:
URL: https://github.com/apache/superset/pull/44470#issuecomment-5769625264
Thanks @rusackas — both directions of that race were real, and you were
right that it isn't a one-line fix while the `catch` doubles as "build vs bail"
for the whole function's return value. Rebased onto current master and pushed;
here's where each point landed.
**The stale initial fetch tearing down an embed a reload already
recovered.** The teardown is now gated on whether anything has taken over the
token chain:
```ts
if (generation === 0) {
teardown("the initial guest token fetch failed");
throw err;
}
log("the initial guest token fetch failed, a reload has taken over:", err);
```
A reload opens a token cycle of its own, and that cycle — not this one —
decides whether the embed lives: it either hands the document in front of the
user a token, or retries until it can. Guarding on `generation` rather than on
"has a token actually arrived" also covers the narrower window where the
reload's own fetch is still in flight when the initial one rejects; tearing
down there would kill the embed microseconds before it recovers.
`embedDashboard` then resolves normally rather than rethrowing.
**The other direction — a valid initial token thrown away for a 10s retry.**
`deliverGuestToken` hands a superseded token over when the current document has
none:
```ts
const ownsChain = gen === generation;
if (ownsChain || !connection.tokenSent) { /* emit */ }
if (ownsChain) { armRefresh(/* … */); }
```
Any valid token gets that document rendering, and only the cycle that owns
the chain arms a timer, so two cycles overlapping across a reload still can't
leave two timers running.
**The `load` listener re-authenticating on any load** — not as harmless as
it looked, so thanks for flagging it. It was minting tokens for documents that
can never use one. The SDK now confirms the handshake before asking the host
for anything: it calls a method the embedded page deliberately does not
implement, and the `Method "..." is not defined` error every version of the
page replies with is itself the acknowledgement. Silence times out at 5s and no
token is fetched. A link in a Markdown chart pointing at a plain Superset URL,
or off site, now costs the host's endpoint nothing, and coming back to the
dashboard re-authenticates as usual.
**The smaller two.** `hostMethods`/`defineHostMethod` no longer use `any` —
the registry is `HostMethod = (args: never) => unknown` with a generic
`defineHostMethod<A extends object>`, so each method keeps its own signature at
the call site and no cast is needed. Tests are flattened to top-level `test()`s
named in the `reload …` / `unmount …` style already used in the file; ten of
them now.
**Heads-up on what else grew here since you reviewed.** Your comments were
against the version that still kept a bare `ourPort`. A `Connection` now
bundles a port with everything scoped to the document behind it, which brought
in two things worth a look: a `get` still in flight when the document leaves is
rejected with `PortClosedError` instead of hanging forever on a reply that can
no longer come (same for `unmount()`), and the theme pushed with
`setThemeConfig`/`setThemeMode` is replayed to the new document after its token
— methods were being replayed but state wasn't, so the dashboard came back in
the default theme.
**On verification.** The unit tests mock `MessageChannel` and `Switchboard`,
which is precisely the layer these bugs live in, so I also built a local test
rig to check the parts that only exist between two documents: a host app and a
stand-in for the embedded page on two origins, driven in headless Chromium
against the built UMD bundle over the real `@superset-ui/switchboard`, no npm
dependencies beyond a chromium binary. 39 checks, including both directions of
this race — it reaches them by holding the first `fetchGuestToken()` open while
the dashboard navigates, which is not otherwise reachable with an endpoint that
answers promptly.
I confirmed each fix by reverting it and watching the matching check go red:
- dropping the `generation === 0` guard → *"the working dashboard is not
torn down"* and *"embedDashboard resolves rather than rejecting"* fail
- dropping `|| !connection.tokenSent` → *"the superseded token is handed
straight to the current document"* fails, bounded to 3s on purpose: a token
that only turns up on the 10s retry is exactly the blank page this is about
It also covers the theme replay, the `PortClosedError` paths, and that a
navigation away from the embedded page mints nothing. I've kept it out of this
diff to hold the review surface to the fix itself, but I'm glad to push it as a
follow-up commit here or as its own PR if you think it earns its place — it's
the only thing I have that exercises the cross-document behaviour for real.
Happy to refresh the PR description too, since it predates the handshake
probe and the `PortClosedError` work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
