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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

thread: don't assume the underlying acceptable priority range #661

Closed
wants to merge 1 commit into from

Conversation

doudou
Copy link
Contributor

@doudou doudou commented Mar 13, 2019

This change is needed by ruby/ruby#2093

Basically, the range of acceptable thread priorities should not be assumed by the spec,
only that invalid priorities are clamped to the acceptable range. The only thing the changed
spec assumes is that 255 and -255 are outside this range.

@eregon
Copy link
Member

eregon commented Mar 13, 2019

I think it's important we test the exact range of valid values, as this is the best ways to ensure the various Ruby implementations support the same range, as mentioned in https://bugs.ruby-lang.org/issues/15438#note-10.

If MRI agrees to change the range, then we can specify it per Ruby version with a version guard (ruby_version_is).

@eregon
Copy link
Member

eregon commented Sep 21, 2019

Closing since the corresponding PR to ruby/ruby was closed: ruby/ruby#2087
Let's resolve https://bugs.ruby-lang.org/issues/15438 first.

FWIW, I think it would be fine to change priority limits if we expose Thread::MIN_PRIORITY and Thread::MAX_PRIORITY.

@eregon eregon closed this Sep 21, 2019
@doudou
Copy link
Contributor Author

doudou commented Sep 23, 2019

ruby/ruby#2087 was closed because of a mixup I've done. The "right" PR is ruby/ruby#2093.

@doudou
Copy link
Contributor Author

doudou commented Sep 23, 2019

In any case, I'm going to modify #2087 to not change the min priority, and propose the min priority change later. I think the bugfix is too important to be blocked by the min priority change.

@eregon
Copy link
Member

eregon commented Sep 23, 2019

👍 Sounds like a good approach to me.

BTW, if you need to modify specs, you can also do it directly in the ruby/ruby PR.

@doudou
Copy link
Contributor Author

doudou commented Sep 23, 2019

BTW, if you need to modify specs, you can also do it directly in the ruby/ruby PR.

Good to know, thanks.

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

Successfully merging this pull request may close these issues.

2 participants