Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix failure when Vite sends many messages in sequence #19340

Merged
merged 2 commits into from
May 9, 2024

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented May 8, 2024

Sending multiple messages at the same time resulted in

java.lang.IllegalStateException: The remote endpoint was in state [TEXT_FULL_WRITING] which is an invalid state for called method

which originates in calling browserSession.getAsyncRemote().sendText a second time before the complete callback for the first call has been called.

Now the getBasicRemote() method is used instead which blocks until the message has been sent.

Sending multiple messages at the same time resulted in

java.lang.IllegalStateException: The remote endpoint was in state [TEXT_FULL_WRITING] which is an invalid state for called method

which originates in calling `browserSession.getAsyncRemote().sendText` a second time before the complete callback for the first call has been called.

Now the getBasicRemote() method is used instead which blocks until the message has been sent.
Copy link

sonarcloud bot commented May 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

github-actions bot commented May 8, 2024

Test Results

1 101 files  ±0  1 101 suites  ±0   1h 21m 58s ⏱️ - 4m 41s
7 004 tests ±0  6 955 ✅ ±0  49 💤 ±0  0 ❌ ±0 
7 370 runs  +3  7 309 ✅ +3  61 💤 ±0  0 ❌ ±0 

Results for commit 5351918. ± Comparison against base commit d55d933.

@mcollovati mcollovati merged commit d7f6fb5 into main May 9, 2024
26 checks passed
@mcollovati mcollovati deleted the fix-vite-proxy-failure branch May 9, 2024 14:55
vaadin-bot pushed a commit that referenced this pull request May 9, 2024
Sending multiple messages at the same time resulted in

java.lang.IllegalStateException: The remote endpoint was in state [TEXT_FULL_WRITING] which is an invalid state for called method

which originates in calling `browserSession.getAsyncRemote().sendText` a second time before the complete callback for the first call has been called.

Now the getBasicRemote() method is used instead which blocks until the message has been sent.
vaadin-bot added a commit that referenced this pull request May 9, 2024
…19343)

Sending multiple messages at the same time resulted in

java.lang.IllegalStateException: The remote endpoint was in state [TEXT_FULL_WRITING] which is an invalid state for called method

which originates in calling `browserSession.getAsyncRemote().sendText` a second time before the complete callback for the first call has been called.

Now the getBasicRemote() method is used instead which blocks until the message has been sent.

Co-authored-by: Artur <artur@vaadin.com>
Comment on lines +57 to 63
browserSession.getNegotiatedSubprotocol(), msg -> {
try {
browserSession.getBasicRemote().sendText(msg);
} catch (IOException e) {
getLogger().debug("Error sending message to browser",
result.getException());
e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The only side effect of this PR (I just checked because of copilot Slack discussion), that we will not have debug logs anymore for the sent messages. Potentially it could be added back in case of need.

browserSession.getNegotiatedSubprotocol(), msg -> {
                    try {
                        browserSession.getBasicRemote().sendText(msg);
                         getLogger().debug("Message sent to browser: {}", msg);
                    } catch (IOException e) {
                        getLogger().debug("Error sending message to browser",
                                result.getException());
                                e);
                    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad. I thought it was intentionally removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Peter for pointing out

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to intervene with your PR review, Marco. I just checked it because we had internal conversations about this issue in the Copilot channel. Thanks for the review and your kindness, as you always show. 🙇 💙

Artur- added a commit that referenced this pull request May 10, 2024
Was unintentionally removed in #19340
mcollovati pushed a commit that referenced this pull request May 10, 2024
vaadin-bot pushed a commit that referenced this pull request May 10, 2024
vaadin-bot added a commit that referenced this pull request May 10, 2024
Was unintentionally removed in #19340

Co-authored-by: Artur <artur@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version.

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

Successfully merging this pull request may close these issues.

None yet

4 participants