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

Equinox Caching: Improved Exception and Shutdown Handling #300

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Equinox Caching: Improved Exception and Shutdown Handling #300

merged 2 commits into from
Feb 5, 2024

Conversation

xpomul
Copy link
Contributor

@xpomul xpomul commented Aug 7, 2023

Sadly, #233 is still not fully solved. The "truncated class file" issue occurs less often now, but it still does occur.
In the last weeks and months, I have analyzed the issue in more depth and applied a fix locally in our codebase. With this now the issue seems to be resolved.

The issue now occurred only between shutdown and restart of the Eclipse Platform which led me to the root cause that the CacheWriter is interrupted when the Platform is shutting down, but if the Platform is fast enough, the CacheWriter might be interrupted or even terminated hard via System.exit() when the Java Process ends.

To compensate, I have added two things:

  1. I have added a join() for the CacheWriter thread with a 5s timeout. Usually it will terminate much faster than that, but you never know
  2. I have pushed the Exception handling down from the main loop of the CacheWriter into the method where writing actually happens. I have also added a check that the file has been written completely. If for any reason (not matching size, interrupt encountered, exception caught) there is a risk of the cache file being corrupt, then we rather delete the cache file. This would lead to a cache miss the next time and thus, some performance penalty, as the class needs to be woven again. But at least we can be more certain that the cache file isn't corrupt.

@xpomul xpomul changed the title Improved exception and shutdown handling Equinox Caching: Improved Exception and Shutdown Handling Aug 7, 2023
@github-actions
Copy link

github-actions bot commented Aug 7, 2023

Test Results

   27 files  +   25     27 suites  +25   11m 11s ⏱️ + 11m 0s
2 170 tests +2 156  2 124 ✅ +2 110  46 💤 +46  0 ❌ ±0 
2 228 runs  +2 200  2 182 ✅ +2 154  46 💤 +46  0 ❌ ±0 

Results for commit ea97c7c. ± Comparison against base commit 850923a.

♻️ This comment has been updated with latest results.

@tjwatson
Copy link
Contributor

tjwatson commented Aug 7, 2023

@martinlippert could you have a review on this?

@martinlippert
Copy link
Contributor

Quick heads-up: I am currently away from work, so can't look at this before Aug 28. If that is fine, happy to look into this then.

Copy link
Contributor

@martinlippert martinlippert 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 to me, thanks a lot for analyzing this in so much depth.

@tjwatson
Copy link
Contributor

I think this will need to wait until after we switch over to 2023-12 development because we are heading into RCs for 2023-09 release.

- Added join for the CacheWriter thread, so it is not terminated hard
- Improved exception and interrupt handling, deleting files if they
  have a risk of being incomplete
@vogella
Copy link
Contributor

vogella commented Oct 13, 2023

Planning to merge after a green build based on @tjwatson comment

@xpomul
Copy link
Contributor Author

xpomul commented Feb 5, 2024

@vogella @tjwatson seems that this PR was forgotten? It's still not merged.
Any chances to get it into 2024-03 at least?

@merks
Copy link
Contributor

merks commented Feb 5, 2024

@xpomul

Yes, it seems to be overlooked. Shall I push the Update branch button to kick off new builds?

@merks
Copy link
Contributor

merks commented Feb 5, 2024

The failures are only the expected familiar ones that are unrelated.

@merks merks merged commit 15708fa into eclipse-equinox:master Feb 5, 2024
24 of 26 checks passed
@merks
Copy link
Contributor

merks commented Feb 5, 2024

@xpomul

Sorry this slipped through the cracks and thank you for the contribution and the general reminder. 🥇

@xpomul
Copy link
Contributor Author

xpomul commented Feb 5, 2024

@merks Thanks, Ed :-)

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

Successfully merging this pull request may close these issues.

5 participants