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: catch exceptions from detach calls #20656

Merged
merged 2 commits into from
Dec 13, 2024
Merged

Conversation

tepi
Copy link
Contributor

@tepi tepi commented Dec 9, 2024

Wraps detach listener calls and onDetach calls into try-catch blocks so all
possible exceptions are handled by session ErrorHandler, and they will not
interrupt running the remaining detach handlers.

Fixes #20577

Copy link

github-actions bot commented Dec 9, 2024

Test Results

1 158 files  ± 0  1 158 suites  ±0   1h 31m 40s ⏱️ -26s
7 521 tests + 1  7 468 ✅ + 1  53 💤 ±0  0 ❌ ±0 
7 866 runs   - 10  7 804 ✅  - 10  62 💤 ±0  0 ❌ ±0 

Results for commit 15ead0c. ± Comparison against base commit 4c6524d.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

ComponentTest.testAttachDetachListeners_parentAttachDetach_childListenersTriggered
could probably be used to create a test that collects the correct amount of events even if one of the listeners throws.

Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

Looks good. The only concern is that this could be some sort of behavioral breaking change, since it doesn't break the application as before, but it "silently" continues the execution (although errors should be logged, unless a custom error handler suppresses them).

It would probably be good to highlight the change in the release notes.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Dec 13, 2024
@mshabarov mshabarov requested a review from caalador December 13, 2024 11:52
@mshabarov mshabarov merged commit a2da336 into main Dec 13, 2024
26 checks passed
@mshabarov mshabarov deleted the fix/wrap-detach-exceptions branch December 13, 2024 13:54
vaadin-bot pushed a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Marco Collovati <[email protected]>
vaadin-bot pushed a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Marco Collovati <[email protected]>
vaadin-bot pushed a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Marco Collovati <[email protected]>
vaadin-bot added a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Teppo Kurki <[email protected]>
Co-authored-by: Marco Collovati <[email protected]>
vaadin-bot added a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Teppo Kurki <[email protected]>
Co-authored-by: Marco Collovati <[email protected]>
vaadin-bot added a commit that referenced this pull request Dec 13, 2024
* fix: catch exceptions from detach calls

* add test

---------

Co-authored-by: Teppo Kurki <[email protected]>
Co-authored-by: Marco Collovati <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants