-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat(eventLoop): the visibility of the shutdown flag for the event loop and optimizations for graceful shutdown #1796
Conversation
@@ -79,6 +89,9 @@ public void execute(Runnable task) { | |||
|
|||
public CompletableFuture<Void> shutdownGracefully() { | |||
shutdown = true; | |||
if (!shutdownCf.isDone()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure concurrent safety, the operations of modifying shutdown
and tasks
need to be safeguarded by a lock. Similarly, other methods that use these two variables need to be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure concurrent safety, the operations of modifying
shutdown
andtasks
need to be safeguarded by a lock. Similarly, other methods that use these two variables need to be modified.
Yes, perhaps we can modify the shutdown from a constant to an AtomicBoolean, utilizing CAS (Compare-And-Swap) to ensure concurrent safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure concurrent safety, the operations of modifying
shutdown
andtasks
need to be safeguarded by a lock. Similarly, other methods that use these two variables need to be modified.
now, the tasks variable has also been modified.
@superhx Perhaps it should only adjust the visibility of the shutdown variable, without introducing a wakeup mechanism, because the worst-case scenario would only involve a wait of 100ms, which is generally acceptable for a graceful shutdown, What do you think? |
import org.slf4j.Logger; | ||
|
||
public class EventLoop extends Thread implements Executor { | ||
private final Logger logger; | ||
private BlockingQueue<Runnable> tasks; | ||
private boolean shutdown = false; | ||
private volatile boolean shutdown; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not see when shutdown = true was set
If it's not completed, I think it's ok to add the shutdown awake. |
@superhx hey, please take a look |
The visibility of the shutdown flag for the event loop and optimizations for graceful shutdown