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

Thread interruption investigation (fixes needed) #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

doctorpangloss
Copy link

These tests show that Thread.currentThread().isInterrupted() doesn't get set to true, as expected, when a virtual thread is interrupted in Vertx.

Additionally, the DefaultVirtualThreadContextTest cannot interrupt Thread.Sleep(), but it's not clear to me if the reason is expected.

@vietj
Copy link
Contributor

vietj commented Dec 19, 2022

thanks

@vietj
Copy link
Contributor

vietj commented Jan 5, 2023

thanks for this interesting PR, after examination here is what I found

First the thread interruption is not correct because in some case (the default scheduler), it will not be executed because the sleep operation will not yield the task execution to the current context, instead it should be done from another plain thread, e.g

      new Thread(() -> {
        Thread.State st;
        while ((st = thisThread.getState()) != Thread.State.WAITING && st != Thread.State.TIMED_WAITING) {
          try {
            Thread.sleep(1);
          } catch (InterruptedException e) {
            throw new RuntimeException(e);
          }
          thisThread.interrupt();
        }
      }).start();

Then the interrupted check is not correct in some tests, because Thread.sleep clears the interrupted status (as per the javadoc).

If I do change both then tests are passing.

NOTE: Thread.sleep will not allow executing other tasks on the context with the default scheduler but it does with the event-loop scheduler. We could remediate this with by adding a sleep method on Async that lets the scheduler be aware of this before performing the actual thread sleep.

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

Successfully merging this pull request may close these issues.

2 participants