Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep thread names under 15 characters #2733

Merged
merged 2 commits into from Dec 12, 2021
Merged

Keep thread names under 15 characters #2733

merged 2 commits into from Dec 12, 2021

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Oct 27, 2021

Description

Keep thread names under 15 characters.

Linux thread names are limited to 15 characters. Trying to set a longer name will just silently do nothing 馃槨

This PR ensures that all threads created by Puma have names that are 15 characters or shorter. Who needs vowels anyway.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented Oct 27, 2021

Makes sense. I'm not a huge fan of some of the changed names here (I would prefer we be as descriptive as possible in the 15 chars available) but that might be bikeshedding.

Could we have a test that uses ps or something to check the thread name?

@ob-stripe
Copy link
Contributor Author

@ob-stripe ob-stripe commented Oct 27, 2021

I'm not a huge fan of some of the changed names here (I would prefer we be as descriptive as possible in the 15 chars available) but that might be bikeshedding.

Totally open to changing names if you have specific suggestions!

Could we have a test that uses ps or something to check the thread name?

There are already a couple of tests that assert some thread names, though they were passing before so I'm not sure how reliable they are. I'll look into adding a Linux-specific test that uses ps or another external tool to get the thread names.

@dentarg dentarg added the waiting-for-changes label Oct 31, 2021
@@ -69,7 +69,7 @@ def start_control

control.binder.parse [str], self, 'Starting control server'

control.run thread_name: 'control'
control.run thread_name: 'ctl'
Copy link
Member

@nateberkopec nateberkopec Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: lets not change any thread names that are always less than 15 characters.

Copy link
Contributor Author

@ob-stripe ob-stripe Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this thread name is also used as a prefix for its accompanying threadpool threads. So if we don't shorten it, we end up with names like puma control tp 001 which is longer than 15 characters.

@ob-stripe
Copy link
Contributor Author

@ob-stripe ob-stripe commented Nov 4, 2021

I added a Linux-only test that uses ps to assert that the thread name is correctly set at the OS level.

@nateberkopec nateberkopec merged commit 5b94b15 into puma:master Dec 12, 2021
44 of 47 checks passed
@dentarg
Copy link
Member

@dentarg dentarg commented Dec 12, 2021

Looks like the added test doesn't work on jruby/truffleruby (but do in their -head versions? 馃 they are allowed to fail) Even fails when running only that test bundle e m test/test_thread_pool.rb:65 (testing in docker run --rm -it -v $PWD:/app -w /app ghcr.io/flavorjones/truffleruby:21.3.0 bash), any ideas @ob-stripe?

@dentarg
Copy link
Member

@dentarg dentarg commented Dec 13, 2021

Ah I guess we can't see the thread names in /proc when we run on the JVM

@dentarg
Copy link
Member

@dentarg dentarg commented Dec 13, 2021

Okay, always one step behind @MSP-Greg (fa178f3) 馃槃

@ob-stripe
Copy link
Contributor Author

@ob-stripe ob-stripe commented Dec 13, 2021

Thanks! I was struggling to get the test suite to pass with all different environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants