ruby-concurrency/concurrent-ruby

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

obulkin opened this issue · 8 comments

obulkin commented

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 commented

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

  • 👍1
obulkin commented

Excellent. I'll get started on one.

obulkin commented

@eregon PR is up now: #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 commented

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

obulkin commented

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 commented

@eregon Bumping again

eregon commented

Merged. Do you need a release?

  • 🎉1
obulkin commented

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