apache / accumulo

Apache Accumulo
https://accumulo.apache.org
Apache License 2.0
1.07k stars 445 forks source link

FATE threads should not be started in the constructor #4609

Open kevinrr888 opened 4 months ago

kevinrr888 commented 4 months ago

Describe the bug Starting threads within a constructor can lead to inconsistencies since the threads may start and access the object before the object is fully constructed. They should instead be started in a separate method that should be called after the object is created. It should probably also be noted in the javadoc for the constructor that the new method should be called after the Fate is created.

Versions (OS, Maven, Java, and others, as appropriate):

dlmarion commented 4 months ago

@kevinrr888 - are you referencing the Runnable created at https://github.com/apache/accumulo/blob/main/core/src/main/java/org/apache/accumulo/core/fate/Fate.java#L227 and https://github.com/apache/accumulo/blob/elasticity/core/src/main/java/org/apache/accumulo/core/fate/Fate.java#L330 that is scheduled to run 3 seconds later?

Looking at this, I think that we should probably be calling scheduleAtFixedRate instead of schedule.

kevinrr888 commented 4 months ago

Yes, and also https://github.com/apache/accumulo/blob/0e3ed1d31fc61a981f7d05928b4696e4d737d4de/core/src/main/java/org/apache/accumulo/core/fate/Fate.java#L356 I'm not sure about scheduleAtFixedRate vs schedule (could very well be better here, I'm just not familiar). It is scheduled to run 3 seconds later and starting the workFinder is done at the end of the constructor, so I don't think either of these are technically a problem. The issue came about from working on https://github.com/apache/accumulo/pull/4524 where I need to start a new thread in Fate. I can't do this from the constructor (e.g., similar to how workFinder is started) without a build failure complaining about starting a thread in the constructor:

[INFO] BugInstance size is 1
[INFO] Error size is 0
[INFO] Total bugs: 1
[ERROR] Medium: new org.apache.accumulo.core.fate.Fate(Object, FateStore, Function, AccumuloConfiguration) invokes Thread.start() [org.apache.accumulo.core.fate.Fate] At Fate.java:[line 353] SC_START_IN_CTOR
[INFO] 

Why this causes a build failure and not workFinder.start(), I'm not sure. Regardless, since I can't do it in the constructor, I call a method after creating the Fate to start the thread. To stay consistent, it would be better if all the threads were started in one method after the creation of Fate. It would also be safer, and I think would be more readable to create the Fate then call something like startup() (it's not obvious that the Fate constructor creates threads unless you look at the code)