RFR (S): 8218483: Crash in "assert(_daemon_threads_count->get_value() > daemon_count) failed: thread count mismatch 5 : 5"

[David Holmes]

PS. I filed:

https://bugs.openjdk.java.net/browse/JDK-8221893
https://bugs.openjdk.java.net/browse/JDK-8221892

for the two bugs you reported.

Thanks,
David

On 3/04/2019 5:14 pm, David Holmes wrote:
> Hi Thomas,
> 
> On 3/04/2019 4:37 pm, Thomas Stüfe wrote:
>> Hi David,
>>
>>
>> On Tue, Apr 2, 2019 at 10:57 PM David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Thomas,
>>
>>     Thanks for taking a look at this.
>>
>>     On 3/04/2019 5:41 am, Thomas Stüfe wrote:
>>      > Hi David,
>>      >
>>      > first thanks for the good analysis!
>>      >
>>      > Is this not a problem with the usage of setDaemon():
>>      >
>>      >
>>     
>> https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#setDaemon(boolean) 
>>
>>      >
>>      > "This method must be invoked before the thread is started."
>>
>>     Not the usage as such, but there is a problem with setDaemon - as 
>> per:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8221657
>>
>>     The test that causes the crash in the VM deliberately tests a case
>>     where
>>     it expects to get the IllegalThreadStateException.
>>
>>      > I think the real solution would be for setDaemon to distinguish
>>     between
>>      > not-yet-started, running and finished. It should not use
>>     isAlive(). It
>>      > should throw an exception if it has been started, regardless of
>>     whether
>>      > it finished already or not.
>>
>>     Yes that fix is needed at the Java level. The use of isAlive()
>>     pre-dates
>>     the existence of Thread.State.
>>
>>     But a change at the Java level may be some time coming given this 
>> is a
>>     day one bug in the spec and implementation of Thread.setDaemon, so I
>>     wanted to address this quickly in the VM as we are seeing these 
>> crashes
>>     in testing.
>>
>>
>> I think a simple patch could be very simply using
>>
>> if (threadStatus != 0)
>>
>> instead of
>>
>> isAlive()
>>
>> in Thread.setDaemon?
> 
> Sure the fix is trivial (plus the method needs to be synchronized), but 
> that assumes that this spec inconsistency:
> 
>       * <p> This method must be invoked before the thread is started.
>       *
>       * @throws  IllegalThreadStateException
>       *          if this thread is {@linkplain #isAlive alive}
> 
> is resolved in favour of the first statement. They may decide that after 
> 25 years it's better to maintain the "not alive" semantics and permit 
> you to modify a terminated thread.
> 
>> We do this in other places in Thread.java too.
>>
>> -- 
>>
>> Also I think it makes sense to scan for similar errors in the code 
>> base (isAlive being used as "has-been-started") and fix those too.
>>
>> For example:
>>
>> ApplicationShutdownHook.java:
>>
>> static synchronized void add(Thread hook) {
>>      if(hooks == null)
>>          throw new IllegalStateException("Shutdown in progress");
>>
>>      if (hook.isAlive())
>>          throw new IllegalArgumentException("Hook already running");
>>
>>      if (hooks.containsKey(hook))
>>          throw new IllegalArgumentException("Hook previously 
>> registered");
>>
>>      hooks.put(hook, hook);
>> }
>>
>> would register a terminated thread as shutdown hook. I found similar 
>> looking code in ThreadPoolExecutor.
> 
> Yeah that's a nasty bug - you can register a shutdown hook that will 
> result in other shutdown hooks not getting started!
> 
>> I really think the jdk would be really the right place to fix this.
> 
> And it may get fixed there eventually. Meanwhile I just want to stop 
> these fairly new assertions from triggering.
> 
> Thanks,
> David
> 
>>
>>     Thanks,
>>     David
>>
>>      > Not sure. Its late, I may not be thinking straight.
>>      >
>>      > Cheers, Thomas
>>      >
>>      >
>>      >
>>      > On Tue, Apr 2, 2019 at 12:33 AM David Holmes
>>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>      > <mailto:david.holmes at oracle.com
>>     <mailto:david.holmes at oracle.com>>> wrote:
>>      >
>>      >     Bug: https://bugs.openjdk.java.net/browse/JDK-8218483
>>      >     webrev: http://cr.openjdk.java.net/~dholmes/8218483/webrev/
>>      >
>>      >     A bug in Thread.setDaemon (JDK-8221657) means that the daemon
>>     state
>>      >     of a
>>      >     thread can change after the thread is !isAlive() at the Java
>>     level. If
>>      >     this happens before the VM call to
>>     ThreadService::remove_thread then we
>>      >     have a situation where we incremented the thread counters
>>     when the
>>      >     thread was not a daemon, and we decrement the thread counters
>>     when the
>>      >     thread is a daemon - and so the counters are out of sync 
>> and the
>>      >     assertion fires.
>>      >
>>      >     The simple fix is to capture the daemon state of the thread
>>     while it is
>>      >     still alive and to pass that through to Threads::remove and 
>> thus
>>      >     ThreadService::remove_thread.
>>      >
>>      >     Testing:
>>      >         - manual test with modified VM (to delay Threads::remove
>>     call)
>>      >     as per
>>      >     the bug report
>>      >         - mach 5 tiers 1-3
>>      >
>>      >     Thanks,
>>      >     David
>>      >
>>