fix: Improve message sender / handler robustness in resync scenarios #13733

TatuLund · 2022-05-11T09:21:51Z

Process resync messages via normal message queue and use semaphor to protect resync process (i.e. do not allow other messages while performing resync)

This PR is adopted from similar fixes for Vaadin 8

vaadin/framework#11791
vaadin/framework#12043
vaadin/framework#12178

This is alternative and possibly more comprehensive fix than

#13727

Fixes #13726

github-actions · 2022-05-11T09:57:04Z

Unit Test Results

  965 files +  24   965 suites +24 45m 16s ⏱️ + 2m 45s
6 274 tests +158 6 226 ✔️ +159 48 💤 ±0 0 ❌ ±0
6 497 runs +156 6 443 ✔️ +157 54 💤 ±0 0 ❌ ±0

Results for commit 7d98249. ± Comparison against base commit 6a5710d.

♻️ This comment has been updated with latest results.

flow-client/src/main/java/com/vaadin/client/communication/MessageHandler.java

flow-client/src/main/java/com/vaadin/client/communication/MessageSender.java

pepijnve · 2022-05-12T11:26:01Z

@TatuLund could you have a look at the changes I made in #13727 as well? I think a variant of that might still be useful. forceMessageHandling still calls RequestResponseTracker#endRequest which may already call MessageSender#sendInvocationsToServer. If that happens the later call to MessageSender#resynchronize will set the flag, but this will not actually get sent since a request may already be in progress. It might be a good idea to set the resynchronizeRequested first before triggering endRequest.

…ageHandler.java Co-authored-by: Artur <[email protected]>

TatuLund · 2022-05-12T11:40:25Z

@pepijnve Yes, I had overlooked that part.

pepijnve · 2022-05-12T13:57:46Z

@TatuLund for my own local testing I've adapted my earlier PR in https://github.com/datadobi/flow/tree/fix13726
Just FYI; feel free to use or implement differently as you see fit.

flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java

mshabarov · 2022-05-13T08:42:16Z

How did you test this patch? Could we use the approach suggested here #13726 (comment) to make an IT test?

pepijnve · 2022-05-17T13:33:01Z

The issue from this ticket is still reproducible with this patch, but we didn't expect it to be resolved 100% thought. Since the patch was proven with the legacy framework, I'll propose to apply it.

@mshabarov there's a further patch to forceMessageHandling that I referenced earlier that I believe would solve this. Could this also be applied since this was the actual issue that Datadobi reported and fixed initially? It would be a shame if this PR, which was meant to supersede my own one, doesn't actually fix the real problem of resync not working reliably.

See 46ebd66

mshabarov

Thanks @pepijnve , I'll have a look how we can combine these two solutions.

pepijnve · 2022-05-17T17:02:02Z

I've rebased https://github.com/datadobi/flow/tree/fix13726 on this PR branch for you. It's one followup commit to Tatu's earlier work. The essence of this commit is that in forceMessageHandling the desire to resynchronise is registered before calling endRequest. If this is not done, endRequest may end up sending out a request itself and that then causes the resynchronize request to fail because a request is already in flight. You end up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java:66.

mshabarov · 2022-05-18T13:45:49Z

@pepijnve thanks a lot for rebasing your changes. I've applied your patch to the current branch, it looks good.

sonarqubecloud · 2022-05-18T14:06:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Artur's comments have been covered.

@pepijnve

…13733) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve

@pepijnve

…13733) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve

@pepijnve

…13733) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve

@pepijnve

…13733) (#13806) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve Co-authored-by: Tatu Lund <[email protected]> Co-authored-by: Artur <[email protected]>

@pepijnve

…13733) (#13804) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve Co-authored-by: Tatu Lund <[email protected]> Co-authored-by: Artur <[email protected]>

@pepijnve

…13733) (#13805) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve Co-authored-by: Tatu Lund <[email protected]> Co-authored-by: Artur <[email protected]>

@pepijnve

…13733) (#13805) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve Co-authored-by: Tatu Lund <[email protected]> Co-authored-by: Artur <[email protected]>

@pepijnve

…13733) (#13805) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve Co-authored-by: Tatu Lund <[email protected]> Co-authored-by: Artur <[email protected]>

@pepijnve

…13733) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve

@pepijnve

…13733) Process re-sync messages via normal message queue and use semaphore to protect re-sync process (i.e. do not allow other messages while performing re-sync). This PR is adopted from similar fixes for Vaadin 8. vaadin/framework#11791 vaadin/framework#12043 vaadin/framework#12178 This also changes the method `forceMessageHandling` in a way that the desire to resynchronise is registered before calling `endRequest`. If this is not done, `endRequest` may end up sending out a request itself and that then causes the re-sync request to fail because a request is already in flight. This ends up throwing an IllegalStateException at com/vaadin/client/communication/RequestResponseTracker.java. Fixes #13726 Co-authored-by: Artur <[email protected]> Co-authored-by: Pepijn Van Eeckhoudt @pepijnve

TatuLund added 2 commits May 11, 2022 12:09

Fix issues with message handler and sender

42e921c

Fix issues with message handler and sender

b0c1023

vaadin-bot added the +0.0.1 label May 11, 2022

Fix

f6f2e92

Artur- previously requested changes May 12, 2022

View reviewed changes

TatuLund and others added 4 commits May 12, 2022 14:33

Expose resynchronizeRequested

c86d3c5

Use resynchronizeRequested in end request

c659a83

Update flow-client/src/main/java/com/vaadin/client/communication/Mess…

3e44642

…ageHandler.java Co-authored-by: Artur <[email protected]>

Fix typo

cf89ec4

vaadin-bot added +1.0.0 and removed +0.0.1 labels May 12, 2022

pepijnve mentioned this pull request May 12, 2022

fix: Avoid IllegalStateException when resyncing #13727

Closed

9 tasks

TatuLund added 9 commits May 12, 2022 15:59

Join ResynchronizationStatus in one enum

925d7d0

Use ResynchronizationState from MessageSender

2eb3d47

Use ResynchronizationState from MessageSender

f215a76

Add import

9aa544c

Add import

f08248d

Fix

356ac9c

Add mock MessageSender

0559fb9

Add mock RequestResponseTracker

f6d3fb7

Add import

70a0f3b

Fix

f5855b3

TatuLund mentioned this pull request May 12, 2022

ComponentRenderer in Grid causes resync to fail #10670

Closed

Code formatting

f27beb4

mshabarov reviewed May 13, 2022

View reviewed changes

flow-client/src/main/java/com/vaadin/client/communication/RequestResponseTracker.java Show resolved Hide resolved

mshabarov added target/23.0 labels May 17, 2022

mshabarov approved these changes May 17, 2022

View reviewed changes

mshabarov requested a review from Artur- May 17, 2022 11:13

Merge branch 'master' into fix13726

fcb22fb

mshabarov requested changes May 17, 2022

View reviewed changes

fix: Ask for resynchronization before end a previous request

b8b393a

chore: fix formatting

7d98249

mshabarov approved these changes May 18, 2022

View reviewed changes

mshabarov merged commit 358ae99 into master May 19, 2022

mshabarov deleted the fix13726 branch May 19, 2022 06:29

vaadin-bot added the cherry-picked-23.1 label May 19, 2022

vaadin-bot added the cherry-picked-23.0 label May 19, 2022

vaadin-bot added the cherry-picked-9.0 label May 19, 2022

TatuLund mentioned this pull request Sep 9, 2022

Resynchronizing UI by client's request #14232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve message sender / handler robustness in resync scenarios #13733

fix: Improve message sender / handler robustness in resync scenarios #13733

TatuLund commented May 11, 2022 •

edited

Loading

github-actions bot commented May 11, 2022 •

edited

Loading

pepijnve commented May 12, 2022

TatuLund commented May 12, 2022

pepijnve commented May 12, 2022 •

edited

Loading

mshabarov commented May 13, 2022

pepijnve commented May 17, 2022 •

edited

Loading

mshabarov left a comment

pepijnve commented May 17, 2022 •

edited

Loading

mshabarov commented May 18, 2022 •

edited

Loading

sonarqubecloud bot commented May 18, 2022

fix: Improve message sender / handler robustness in resync scenarios #13733

fix: Improve message sender / handler robustness in resync scenarios #13733

Conversation

TatuLund commented May 11, 2022 • edited Loading

github-actions bot commented May 11, 2022 • edited Loading

Unit Test Results

pepijnve commented May 12, 2022

TatuLund commented May 12, 2022

pepijnve commented May 12, 2022 • edited Loading

mshabarov commented May 13, 2022

pepijnve commented May 17, 2022 • edited Loading

mshabarov left a comment

Choose a reason for hiding this comment

pepijnve commented May 17, 2022 • edited Loading

mshabarov commented May 18, 2022 • edited Loading

sonarqubecloud bot commented May 18, 2022

TatuLund commented May 11, 2022 •

edited

Loading

github-actions bot commented May 11, 2022 •

edited

Loading

pepijnve commented May 12, 2022 •

edited

Loading

pepijnve commented May 17, 2022 •

edited

Loading

pepijnve commented May 17, 2022 •

edited

Loading

mshabarov commented May 18, 2022 •

edited

Loading