DaemonThreadFactory creating new Java thread factory each time it creates a new thread

[obulkin]

Background Info

• Ruby implementation: JRuby
• concurrent-ruby version: >= 1.1.10, earlier versions may be affected as well
• concurrent-ruby-ext installed: no
• concurrent-ruby-edge used: no

Issue

Concurrent::DaemonThreadFactory#newThread calls defaultThreadFactory() each time it generates a new thread, which creates a new Java ThreadFactory object each time. This is clearly at odds with intended ThreadFactory usage and is (potentially) problematic in a few ways:

• It leads to thread names of the form pool-X-thread-Y, where Y is always 1 while X is different for each thread. This is confusing as it suggests that the Concurrent Ruby thread pool is not actually pooling and reusing threads.
• This does not seem to be happening in the Concurrent Ruby code, but if a reference to each ThreadFactory instance is retained somewhere, the result will be a memory leak.
• Threads created by a ThreadFactory instance normally belong to the same Java thread group, which has permissions implications and may cause errors in code that expects all the threads in a Concurrent Ruby thread pool to belong to the same Java thread group.

Proposed Solution

Call defaultThreadFactory() once in Concurrent::DaemonThreadFactory#initialize, store the resulting factory in an instance variable, and reuse it as needed in Concurrent::DaemonThreadFactory#newThread.

Happy to create a PR with the fix if this seems like an acceptable solution.

[eregon]

Thanks for the report. Yes, that makes sense, a PR would be helpful.

[obulkin]

Excellent. I'll get started on one.

[obulkin]

@eregon PR is up now: https://github.com/ruby-concurrency/concurrent-ruby/pull/1009

The CI failure seems like a flaky test since the test case doesn't seem related to my changes and passed under JRuby.

[obulkin]

@eregon Checking in on this. I recommitted my changes to force a new test run, and everything is green this time.

[obulkin]

Checking in. Please let me know if I can do anything to move this along. The PR is very minimal and should be quick to review.

[obulkin]

@eregon Bumping again

[eregon]

Merged. Do you need a release?

[obulkin]

Thanks! No urgent need for a release if there's something else in the pipeline that you'd prefer to bundle it with.