Added defensive code for scenario where thread id <= 0

[dhoard]

Signed-off-by: Doug Hoard doug.hoard@gmail.com

[dhoard]

PR to resolve #784 which will ultimately resolves prometheus/jmx_exporter#601

@fstab please review

[fstab]

Hi Doug, thanks a lot for the PR. Could you elaborate what this is about? As far as I can see threadBean.getAllThreadIds() iterates through the threads and calls thread.getId() for each thread.

From the Javadoc of thread.getId():

...the thread ID is a positive long number...

So I'm wondering where non-positive thread IDs could come from?

[dhoard]

@fstab

TL;DR

ZooKeeper class QuorumPeer derives from Thread and overrides getId() based on the ZooKeeper node id (myid = 0). (I don't have the history as to why ZooKeeper chose to override the thread id... but it's been in their code code for at least 4+ years.)

https://issues.apache.org/jira/browse/ZOOKEEPER-4460

I feel that it shouldn't break ThreadExports.

Detailed Analysis

prometheus/jmx_exporter#601

ThreadExports Test Case

  @Test
  public void testThreadId0() {
    final CountDownLatch countDownLatch = new CountDownLatch(1);

    try {
      Runnable runnable = new Runnable() {
        @Override
        public void run() {
          try {
            countDownLatch.await();
          } catch (InterruptedException e) {
            // DO NOTHING
          }
        }
      };

      Thread thread = new MyThread(runnable);
      thread.setDaemon(true);
      thread.start();

      ThreadExports threadExports = new ThreadExports();
      threadExports.collect();
    } finally {
      countDownLatch.countDown();
    }
  }

  class MyThread extends Thread {

    public MyThread(Runnable runnable) {
      super(runnable);
    }

    public long getId() {
      return 0;
    }
  }

Test Case Result (without the fix)

testThreadId0(io.prometheus.client.hotspot.ThreadExportsTest)  Time elapsed: 0.142 sec  <<< ERROR!
java.lang.IllegalArgumentException: Invalid thread ID parameter: 0
	at java.management/sun.management.ThreadImpl.verifyThreadId(ThreadImpl.java:165)
	at java.management/sun.management.ThreadImpl.verifyThreadIds(ThreadImpl.java:174)
	at java.management/sun.management.ThreadImpl.getThreadInfo(ThreadImpl.java:180)
	at io.prometheus.client.hotspot.ThreadExports.getThreadStateCountMap(ThreadExports.java:146)
	at io.prometheus.client.hotspot.ThreadExports.addThreadMetrics(ThreadExports.java:113)
	at io.prometheus.client.hotspot.ThreadExports.collect(ThreadExports.java:177)
	at io.prometheus.client.hotspot.ThreadExports.collect(ThreadExports.java:171)
	at io.prometheus.client.hotspot.ThreadExportsTest.testThreadId0(ThreadExportsTest.java:119)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:189)
	at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:165)
	at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:85)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:115)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:75)

Update: Added the test case to ThreadExports and link to ZooKeeper issue.

[fstab]

Thanks a lot for the explanation. That's unexpected. I agree we should handle this and not just fail in that case. I left a few review comments. Thanks for picking this up!

[fstab]

Thank you!