Restore the interrupted status for javascript.background.DefaultJavaScriptExecutor.killThread

[guang-hou]

In the killThread method, instead of ignoring the InterruptedException, should it restore the interrupted status using Thread.currentThread().interrupt()?

I am using a threadpool to run multiple webClient instance, the interrupted status is needed to perform some action. The current version seems to swallow the thread interruption status.

@SuppressWarnings("deprecation")
   private void killThread() {
       ...
       try {
           eventLoopThread_.interrupt();
           eventLoopThread_.join(10_000);
       }
       catch (final InterruptedException e) {
           LOG.warn("InterruptedException while waiting for the eventLoop thread to join ", e);
           // ignore, this doesn't matter, we want to stop it
       }
       ...
   }

[rbri]

@guang-hou can you please add some more details about this - i'm not sure that i got your point here.

[guang-hou]

With current version, when a thread is running multiple methods, such as the one below. When the thread is interrupted from outside, the doSomething() will not run because catching the InterruptedException reset the interruption status.

withInOneChildThread {
  htmlunitMethods()
  
  someOtherMethod() {
      if (Thread.currentThread().isInterrupted()) {
         doSomething();
      }
   }
}

We can restore the interruption status by adding Thread.currentThread().interrupt(); . This will restore the interruption status so doSomething() can run.

if (this.eventLoopThread_ != null) {
    try {
        this.eventLoopThread_.interrupt();
        this.eventLoopThread_.join(10000L);
    } catch (InterruptedException e) {
        LOG.warn("InterruptedException while waiting for the eventLoop thread to join ", e);
        Thread.currentThread().interrupt();   // add this line
    }
}

[rbri]

@guang-hou ah ok, now i got your point - sorry.

Will try to make a test case out of this and add your fix (maybe also at other places). Hope i can do this today before the release.

[rbri]

@guang-hou
Sorry but i was not able to write a test case for this, will work on this during the next days.

But in your case the eventLoopThread_ was interrupted - i think we have to do a

eventLoopThread_.interrupt();

[niloc132]

@rbri I think the code as written is correct, since the try block is already interrupting the eventLoopThread_, but then catching the interrupt ex in the join() call - when an InterruptedException is caught (i.e. on currentThread()), it unsets the interrupt flag. So, the catch should re-set that flag on the current thread (via currentThread().interrupt()).

It isn't always required/expected to re-set the interrupt status, if the current thread has some sort of contract with whatever created it to handle interrupts in a specific way - but since this code doesn't know what that contract might be and doesn't expect to catch an InterruptedException, it would be more appropriate to re-set the interrupt flag, and let the caller read it.

Consider the case where this code was run on some thread, and the main thread has gone around and asked each thread to stop by interrupting it. In each thread, the application author might have periodic checks of the current thread's interrupted status. If the main thread called interrupt, and the interrupt happened during the join(), then the catch would have un-set it, so when killThread() returned, the calling code wouldn't see that interrupt status as it should have.

[niloc132]

https://stackoverflow.com/a/3976527/860630 seems to have a pretty good summary of best practices here. Note that this is why it is almost always wrong to have a catch (Exception e) in Java code without an earlier block for InterruptedException to handle this case correctly.

[rbri]

Was not able to write a really good (and stable) test case for this but the fix is not in.
@guang-hou please have a look....

[rbri]

A first 3.5.0-SNAPSHOT is available including your fix.

[rbri]

@guang-hou
Thanks for the report - hope this works for you now.

[guang-hou]

Yes, I tested the new version and it works. Thanks