fix(flow-client): ignore a response that has no request to end - #25469
Open
totally-not-ai[bot] wants to merge 3 commits into
Open
fix(flow-client): ignore a response that has no request to end#25469totally-not-ai[bot] wants to merge 3 commits into
totally-not-ai[bot] wants to merge 3 commits into
Conversation
## What
`MessageHandler.endRequestIfResponse` now ends the request only when one
is active, both in the GWT client and in the TypeScript port. When there
is none it logs a debug message and still stops the loading indicator.
## Why
`RequestResponseTracker.endRequest()` throws
`IllegalStateException("endRequest called when no request is active")`,
and the already-seen branch of `handleJSON` calls `endRequestIfResponse`
for a message it has decided to ignore. A single client message opens
exactly one request, so when the server delivers a second copy of a
response the client already handled, the first copy ends the request and
the second one hits the unguarded `endRequest()`. The throw only reaches
the uncaught handler, which logs it, so the loading indicator is left
without its stop for that message.
Fixes #25467
## Tests
`MessageHandlerTests.ts` now models the tracker as the real one behaves:
a request is active when the response arrives, `endRequest` clears it and
throws when nothing is active. A new case re-delivers the same response
and asserts it is ignored without a second `endRequest`, while the
loading indicator is still stopped. Without the fix that case fails with
`endRequest called when no request is active`.
## What `GwtMessageHandlerTest.TestRequestResponseTracker` now models the real tracker: a request is active when the response arrives, `endRequest` clears it and throws when there is none, and the calls are counted. A new case delivers the same response twice and asserts the request is ended once. Also reverts the `flow-client/package-lock.json` churn that an `npm install` in the previous commit picked up. ## Why The stub only overrode `endRequest` and inherited `hasActiveRequest`, which is `false` because `startRequest` is never called, so after the guard every response in the Java suite took the "no request is active" branch and `endRequest` was never reached at all. The lockfile diff only dropped the `libc` optional-dependency metadata that newer npm writes, and is unrelated to this fix. ## Tests The GWT suite is not wired into the default build (`gwt.test.pattern` is undefined, so the `gwt-test` execution matches no test), and running it explicitly fails in this environment before the change as well: all eight cases, including untouched ones, error out while compiling the JUnit module with `Rebind result AtmospherePushConnection$Factory could not be found`. The new case is therefore verified only by javac and by its TypeScript counterpart in `MessageHandlerTests.ts`, which does run and which fails without the guard.
Artur-
reviewed
Sep 3, 2026
| } else { | ||
| // No request to end, e.g. a duplicate of a response that already ended | ||
| // it. endRequest would throw for that. | ||
| Console.debug('Received a response while no request is active. Not ending a request.'); |
Member
There was a problem hiding this comment.
Why should this point out that it's not ending a request instead of just logging what happened? It's not updating the database either
Contributor
Author
There was a problem hiding this comment.
Dropped the second sentence, in both clients: the message is now just Received a response while no request is active.
The debug message in `endRequestIfResponse` now states that a response arrived while no request is active, without also listing what the client did not do in response to it.
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
MessageHandler.endRequestIfResponsenow ends the request only when one is active, both in the GWT client (MessageHandler.java) and in the TypeScript port (MessageHandler.ts). When there is no active request it logs a debug message and still stops the loading indicator.Why
RequestResponseTracker.endRequest()throwsIllegalStateException("endRequest called when no request is active"), and the already-seen branch ofhandleJSONcallsendRequestIfResponsefor a message it has decided to ignore.A single client message opens exactly one request, so when the server delivers a second copy of a response the client already handled, the first copy ends the request and the second one hits the unguarded
endRequest(). The throw only reaches the uncaught handler, which logs it, sostopLoading()is never called and the loading indicator is left running for that message.Fixes #25467
Tests
MessageHandlerTests.tsnow models the tracker as the real one behaves: a request is active when the response arrives,endRequestclears it and throws when nothing is active. A new case re-delivers the same response and asserts it is ignored without a secondendRequest, while the loading indicator is still stopped. Without the fix that case fails withendRequest called when no request is active.GwtMessageHandlerTest.TestRequestResponseTrackerwas likewise made to model the real tracker (active request on arrival,endRequestclears it and throws when there is none, calls counted), and a new case delivers the same response twice and asserts the request is ended once. Previously the stub only overrodeendRequestand inheritedhasActiveRequest, which isfalsebecausestartRequestis never called — so after the guard every response in the Java suite would have taken the "no request is active" branch andendRequestwould never have been exercised at all.Note on the GWT suite: it is not wired into the default build (
gwt.test.patternis undefined, so thegwt-testexecution matches no test), and running it explicitly fails in this environment before this change as well — all eight cases, including untouched ones, error out while compiling the JUnit module withRebind result AtmospherePushConnection$Factory could not be found. The new Java case is therefore verified only byjavacand by its TypeScript counterpart, which does run and which fails without the guard.