• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Fixed
  • DSO:L1
  • wharley
  • Reporter: cdennis
  • May 07, 2009
  • 0
  • Watchers: 1
  • August 20, 2009
  • May 21, 2009

Description

We key global thread id objects in each L1 against the return of getId for associated local thread object. getId is not a final method. Thread subclasses are free to override and return whatever they like (including a static value). I suspect (although it is not yet confirmed) that this has already been exposed by at least one forum user. In addition this map is not ever scrubbed of dead threads, and even in the absence of bizarre subclasses the JVM can recycle the ids of dead threads.

Note that we assert this mapping to be unique for any given thread - so breaking subclasses or JVM id reuse will trigger an assertion error.

Is there some reason we are not using a WeakHashMap with the threads themselves as the keys? This would fix the id bug and also be self cleaning.

Comments

Chris Dennis 2009-05-08

Confirmed case of a subclass causing this @ http://forums.terracotta.org/forums/posts/list/0/2116.page

Chris Dennis 2009-05-08

Sun are aware of the non-final method being a bug but are not going to change it to avoid breaking backwards compatibility. http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6346938

Alex Miller 2009-05-08

For now, target Rivera. Depending on scope of fix, might want to roll back to 3.0 branch for next 3.0.x.

Walter Harley 2009-05-18

Our main use of thread IDs, in lock management, uses TC ThreadIds, which are associated to a VM thread via a ThreadLocal (in ThreadIdManagerImpl). Thus, we are using actual Thread objects, not the getId(), for most of what we do.

The ThreadIdMap seems to only be used for the sake of getting thread dumps; at least that’s the only caller of ThreadIdMap.getTCThreadID() that I can find. Unfortunately in that case the VM thread ID that we’re looking up on is coming from the ThreadMXBean, which does not provide a way to get actual threads, only thread IDs. This is a Java class, not one of ours.

I’m inclined to say that the assert in ThreadIDMapJdk15.addTCThreadID() is incorrect, that is, that we should silently tolerate duplicated ids. If someone overrides getId() and fails to honor the contract in its javadoc (“The thread ID is unique during its lifetime. When a thread is terminated, this thread ID may be reused”), they’ll get bogus thread dumps, but not broken locking. If they are simply recycling thread IDs, then no harm done.

But the map can grow forever. We could make the map be <Thread, ThreadId> if in ThreadDumpUtilJdk15 we used Thread.enumerate() instead of ThreadMXBean.getAllThreadIds(). I can’t tell from the docs whether these are identical: enumerate() returns only threads in the current thread’s ThreadGroup, but getAllThreadIds() looks like it is less restrictive.

Another idea would be to create a WeakReference to the Thread at the same time that we put its ID into the map. Then when we dequeue the references we can remove the corresponding IDs from the map. This has two problems: first, where do we check the queue? On a separate cleanup thread, or inside all the methods that access the map, or … ? Second, it is not robust against a badly overridden getThreadId() method that fails to produce unique thread IDs: collisions will cause live threads to be removed from the map.

Walter Harley 2009-05-18

Looks like it is possible to walk up the thread group tree with ThreadGroup.getParent(), to find the root thread group; calling enumerate() on that should give the same result as ThreadMXBean.getAllThreadIds(). I’ll explore this approach.

Walter Harley 2009-05-20

The ThreadMXBean (the VM’s way of reporting thread info, that we use to obtain locks, monitors, and thread state, as well as the stack trace) has a getThreadInfo(id) method; but if you pass in the id of a Thread that has overridden getId(), it returns null - instead, if you pass in the original, non-overridden id, you get the correct thread info. It seems that the VM itself does not cope correctly with overriding Thread.getId().

I think we should prevent the ThreadIdMap from growing without bound, and we should gracefully handle (ie, no crash, no assertion) the case where thread ID has been overridden, but I do not think it is worth further dev effort to try to correctly report full thread dump info in that case. Instead, we should just doc the gotcha.

Walter Harley 2009-05-21

Fixed in trunk with 12756. We could merge this into 3.0.x; it does change the ThreadIDMap interface but that is not in a -api project.

We certainly want to advise users to NOT override Thread.getId(), as doing so exposes JVM inconsistencies (specifically in the ThreadMXBean). If they do so anyway, our locking will be unaffected; our thread dumps may not be able to get full information about thread state, but they will still contain complete stack traces and information about TC locks.

Walter Harley 2009-05-21

Created DOC-614 for the doc issue.

Kalai Kannaiyan 2009-08-12

Verified the changes made on trunk with svn rev 12755, 12756 and 12758.