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

[5.2.x] JGRP-2835 Do not log warn during normal shutdown. #837

Open
wants to merge 1 commit into
base: 5.2.x
Choose a base branch
from

Conversation

pferraro
Copy link
Contributor

@belaban
Copy link
Owner

belaban commented Sep 18, 2024

Hmm, I don't like this solution. Thread interruption doesn't necessarily always mean loop termination, as threads can be spuriously interrupted. IIRC, I once had while(running && !Thread.isInterrupted()), but changed it for the above reason and also to avoid 2 reads on volatile variables (potentially leading to cache refreshes).
I'll take a look, but not high prio since this is not causing issues.

@pferraro
Copy link
Contributor Author

Thread interruption doesn't necessarily always mean loop termination.

In general, yes - though, in this case, the thread executing the run() method is known to be the thread created during start().

I don't see how you can avoid a volatile read though - since the only way to reliably exit the loop from a condition set from a different thread is to perform a volatile read.

@belaban
Copy link
Owner

belaban commented Sep 18, 2024

Yes, but currently, it is only one read (running) versus 2 (running plus Thread.interrupted).
This is a critical piece of code, so I'll have to run perftests after any change... Stay tuned!

@pferraro
Copy link
Contributor Author

Yes, but currently, it is only one read (running) versus 2 (running plus Thread.interrupted).

I assume you are talking about the run() method, right?
If so, unless I am missing something, in my proposed change there is also only 1 volatile read per iteration, no?

This is a critical piece of code, so I'll have to run perftests after any change.

Makes sense.

@belaban
Copy link
Owner

belaban commented Sep 18, 2024

Yes, only 1 read (Thread.interrupted), but I was worried that that's not enough and would have combined it with a read on running, so 2 volatile reads...
Again, it boils down to perf, and I'll double and triple check perf before committing such a change.

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.

2 participants