Do not visit gc threads during shutdown #228
Conversation
|
👋 Welcome back wkemper! A progress list of the required criteria for merging this PR into |
|
@earthling-amzn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 104 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit d367909.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@earthling-amzn Pushed as commit d367909. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
| if (_shenandoah_policy->is_at_shutdown()) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Some related questions so I can understand the shutdown sequence a bit better, and what caused the issue that @kdnilsen recorded in the ticket:
- I suppose we have determined that the shutdown sequence doesn't itself need to iterate over these GC-related threads to do anything.
- was the original issue because one of the threads gone by the time we started iterating over it with the closure below?
- what prevents a race between the check above and the exit of the threads subject to iteration below, so that some of the threads may be asynchronously gone between the check above and the iteration over that thread below?
Thanks!
There was a problem hiding this comment.
- The shutdown sequence doesn't need to iterate over the threads.
- The crashing thread wasn't "gone", but it's termination had been requested. It crashed in a call to get the thread's clock time (pthread_getcpuclockid). The thread's native handle should still have been valid. I didn't fully root cause the crash.
- In this case, it was the control thread itself that was trying to iterate the threads. The fix here will stop the control thread from starting an iteration of the threads after the shutdown sequence has begun, but it is not so robust to cover a shut down during an iteration of the threads.
There was a problem hiding this comment.
- The shutdown sequence doesn't need to iterate over the threads.
Good!
- The crashing thread wasn't "gone", but it's termination had been requested. It crashed in a call to get the thread's clock time (pthread_getcpuclockid). The thread's native handle should still have been valid. I didn't fully root cause the crash.
I assume you meant "the GC thread that the control thread was iterating over wasn't gone, but its termination had been requested"? I'd consider that as good as gone, absent any interlocking between the iteration and the termination of the target thread.
- In this case, it was the control thread itself that was trying to iterate the threads. The fix here will stop the control thread from starting an iteration of the threads after the shutdown sequence has begun, but it is not so robust to cover a shut down during an iteration of the threads.
May be one then needs a handshake/interlock with the control thread during the shutdown sequence if it's the only thread that iterates over these threads. (Or their termination could be delegated to the control thread, which is morally equivalent, and perhaps cleaner.) Is the case then that you have by your change above substantially narrowed the window of vulnerability, but not completely closed it? Or did I misunderstand?
There was a problem hiding this comment.
The core file I had was on /tmp and was lost in one of the power outages last week. I meant that the control thread's termination was requested and it was the control thread itself that was iterating the other threads. I don't recall which visited thread caused the crash. As I think on this, I'd like to make another small change to make it more robust.
Yes, that's fair. This change narrows the window, but doesn't completely close it.
Discovered a strange crash during shutdown while working on jtreg tests.
Progress
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/shenandoah.git pull/228/head:pull/228$ git checkout pull/228Update a local copy of the PR:
$ git checkout pull/228$ git pull https://git.openjdk.org/shenandoah.git pull/228/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 228View PR using the GUI difftool:
$ git pr show -t 228Using diff file
Download this PR as a diff file:
https://git.openjdk.org/shenandoah/pull/228.diff