Skip to content

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

Open

Description

@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.

Activity

  1. eregon commented on Aug 24, 2023

    @eregon
    Member

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

  2. obulkin commented on Aug 24, 2023

    @obulkin
    ContributorAuthor

    Excellent. I'll get started on one.

  3. obulkin commented on Aug 29, 2023

    @obulkin
    ContributorAuthor

    @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.

  4. obulkin commented on Sep 6, 2023

    @obulkin
    ContributorAuthor

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

  5. obulkin commented on Sep 20, 2023

    @obulkin
    ContributorAuthor

    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.

  6. obulkin commented on Oct 18, 2023

    @obulkin
    ContributorAuthor

    @eregon Bumping again

  7. eregon commented on Oct 21, 2023

    @eregon
    Member

    Merged. Do you need a release?

  8. obulkin commented on Oct 23, 2023

    @obulkin
    ContributorAuthor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions