Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

New 0.4.9 ThreadFactory creating daemon threads #95

Closed
aslakhellesoy opened this Issue Jun 27, 2012 · 11 comments

Comments

Projects
None yet
3 participants
Owner

aslakhellesoy commented Jun 27, 2012

This came from one of my colleagues. @s1monw any comments?


42c0fad

This change is seemingly harmless in that it is simply adding names to various webbit threads.
The problem is that the ThreadFactory implementation is creating daemon threads. For apps that are relying on the webbit threads to be user threads in order to keep the app alive, they will simply exit after calling start.

I’m pretty sure this is not the intention since every sample creates a WebServer and starts it. To see the behavior, choose any of the webbit sample apps, run it, and observe that it exits out and doesn’t stay alive. Since NamedThreadFactory seems to have been copied from another project, I’m not sure if the proper fix is the flip isDaemon(false) in NamedThreadFactory.newThread(). I suppose we could just remove the “copied from Lucene” comment so there is no confusion. Let me know how we should go about fixing this.

Owner

aslakhellesoy commented Jun 27, 2012

The JavaDoc of the orignally submitted NamedThreadFactory claims that it yields the same semantics as the thread factory returned by Executors.defaultThreadFactory().

This was not the case. A great example of why comments are evil. Here is what probably happened:

  1. Someone added this class (with correct comments) to Lucene with thread semantics identical to Executors.defaultThreadFactory()
  2. Someone decided threads needed to be daemons (fine), but forgot to modify the (now incorrect) comment
  3. @s1monw copies the code into Webbit, assuming all is OK and sends a pull request
  4. I gloss over the new code, spot the comment about similar thread sematics, run the tests and since all is green - merge and release.

Don't comment code!

Contributor

s1monw commented Jun 27, 2012

ha! that is all my fault. I wrote that class a couple of years ago and indeed its comment is wrong! I will fix here and in Lucene!

Contributor

s1monw commented Jun 27, 2012

looking at https://svn.apache.org/repos/asf/lucene/dev/trunk/lucene/core/src/java/org/apache/lucene/util/NamedThreadFactory.java I must have messed this up somehow while copying. I have no idea how this happened but the lucene one sets setDeamon to false. I didn't do that intentionally at all. Sorry for the hassle!

Owner

aslakhellesoy commented Jun 27, 2012

@s1monw I'm not sure what you mean. The lucene one doesn't have similar semantics to the default one in the JDK either.

Owner

aslakhellesoy commented Jun 27, 2012

So you just copied something that was already broken/lying. No problem - I already released Webbit 0.4.11 with a fix.

Contributor

s1monw commented Jun 27, 2012

@aslakhellesoy afaik the problem here is that the code I had in the pull req. was setting setDaemon to true which is obviously wrong. But that is not in the lucene version and I have no idea why the hack it was in the code. I must have accidentally change that. That is all I am saying

Owner

aslakhellesoy commented Jun 27, 2012

Oh I see. I wasn't looking carefully enough. Here is what the DefaultThreadFactory does:

if (t.isDaemon())
    t.setDaemon(false);
if (t.getPriority() != Thread.NORM_PRIORITY)
    t.setPriority(Thread.NORM_PRIORITY);

The NamedThreadFactory didn't have those if statements (it does now), but I guess the behaviour remains roughly the same with or without them.

Owner

joewalnes commented Jun 27, 2012

A few thoughts, in no particular order:


It seems the intent of the original commit was solely to set the name of the threads. I'm not sure how we got into the realm of SecurityManagers, ThreadGroups, priorities and daemons.

A much simpler implementation would be something like:

/**
 * Wraps a ThreadFactory and assigns a name to each newly created thread, that includes two
 * counters: one to indicate which ThreadFactory instance this is, and another for the thread
 * created by the factory.
 */
public class NamingThreadFactory implements ThreadFactory {

  private static volatile int factoryCount;

  private volatile int threadCount;
  private final ThreadFactory factory;
  private final String prefix;

  public NamingThreadFactory(ThreadFactory factory, String prefix) {
    this.factory = factory;
    this.prefix = prefix;
    this.factoryCount++;
  }

  public NamingThreadFactory(String prefix) {
    this(Executors.defaultThreadFactory(), prefix);
  }

  @Override
  public Thread newThread(Runnable r) {
    this.threadCount++;
    Thread thread = factory.newThread(r);
    thread.setName(threadName());
    return thread;
  }

  /**
   * Override this method to customize thread name.
   */
  protected String threadName() {
    return String.format("%s-%d-%d-thread", prefix, factoryCount, poolCount); 
  }
}

Another such approach would be do not even bother with a custom ThreadFactory and simply call Thread.setName() in the relevant places in Webbit.


I am uncomfortable with the approach of copying code from another project, modifying it, then deleting references to its original license. A few years down the line, when Webbit has been acquired by a mega large company (let's call it Poogle), and ASF has been acquired by another large company (let's call it Doracle) - this would be a prime law suit.

I would prefer that this class was deleted altogether to avoid any ambiguities. Replace it with any of the approaches above, or something else.


On the assertions "Don't comment code" and "comments are evil", I have to disagree, in this case as the comments were actually documentation. I am all for documentation if it helps to improve the understandability of code and make life easier for the user. But documentation is part of a bigger system, and when changes are made, it also need to be update in the same manner as unit-tests, changelogs, etc. Documentation are part of the code of the system and should be reviewed in a similar manner.

I deem Webbit an infrastructure library, with many systems (outside of our control) building on it. This carries a tax, one of those is ensuring that documentation is meaningful and up to date. The lower down the stack you go, the higher the tax is - imagine if malloc() had no documentation.

So my assertions is: "Documentation is good if it provide value to the user and is kept accurate."


I think we could do a lot of work to improve the code review process of Webbit to catch things like this. One of the biggest problems is that I have expectations in my head that I have not communicated to the rest of the team. I pin this on me and I am open to ideas of how to improve it.

Owner

aslakhellesoy commented Jun 27, 2012

I think you wrote most of this without checking my latest commits...

I'm not sure how we got into the realm of SecurityManagers, ThreadGroups, priorities and daemons.

This is what java.util.concurrent.Executors.DefaultThreadFactory does. The NamedThreadFactory is a modified copy of that code. Clearly using delgation as you suggested (and I just pushed) is a better way to go.

I am uncomfortable with the approach of copying code from another project...

Agreed. The copied code is gone now. I remember all the hassle with QDox a few years ago, signing legal docs etc.

I would prefer that this class was deleted altogether to avoid any ambiguities.

Even after my recent changes? See 227861a and https://github.com/webbit/webbit/blob/227861aaef470972903d41088c4f7b8584cf1175/src/main/java/org/webbitserver/helpers/NamedThreadFactory.java

Joe, if you'd rather use the code you pasted here I'll leave it to you to overwrite it.

On the assertions "Don't comment code" and "comments are evil", I have to disagree

I didn't really mean that. As you well know I comment my code too when necessary. It just seemed to be the culprit in this case. (Turns out it was not).

I think we could do a lot of work to improve the code review process of Webbit to catch things like this.

I also take responsibility for this. I was the one who merged it in, and if I had taken a little more time to review it I would have caught some of the problems.

Owner

aslakhellesoy commented Jun 27, 2012

@joewalnes allright - added your NamingThreadFactory and completely removed the other one to avoid any legal issues. See d2f1f2e

Owner

joewalnes commented Jun 27, 2012

Thanks Aslak.

I think my code could also do with a bit more reviewing. There are some
issues with it...

On Wed, Jun 27, 2012 at 8:52 AM, Aslak Hellesøy <
reply@reply.github.com

wrote:

@joewalnes allright - added your NamingThreadFactory and completely
removed the other one to avoid any legal issues.


Reply to this email directly or view it on GitHub:
#95 (comment)

@aslakhellesoy aslakhellesoy added a commit to aslakhellesoy/webbit that referenced this issue Jun 27, 2012

@aslakhellesoy aslakhellesoy Use AtomicInteger instead of volatile int. For #95. f980058
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment