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

Potential bug in internal/runtime/ThreadService? #4844

Closed
nglorioso opened this Issue Nov 9, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@nglorioso
Contributor

nglorioso commented Nov 9, 2017

if (rubyThreadMap.size() >= 0) polling = true;

Here, and a bit below, a Map's (a synchronized WeakHashMap) size is checked as being >= 0 to twiddle the state of the polling field.

I'm pretty sure WeakHashMap would never return a size < 0. I haven't fully grasped the rest of the file (should this only be set iff there's more than 1 active thread?), but certainly if (map.size() >= 0) is always going to be true.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 9, 2017

Member

Yeah seems like this should be > 0.

Luckily, it turns out we don't use the polling field for anything right now. I remember at one point wanting to add this feature, where like MRI we could turn off certain thread events (like kill, raise across threads) when there's only a main thread. Ultimately checking this boolean turns out to be as expensive as polling anyway, so it never went anywhere.

I'll leave the flag, for API and info purposes, but I'll fix it to be > 0.

Member

headius commented Nov 9, 2017

Yeah seems like this should be > 0.

Luckily, it turns out we don't use the polling field for anything right now. I remember at one point wanting to add this feature, where like MRI we could turn off certain thread events (like kill, raise across threads) when there's only a main thread. Ultimately checking this boolean turns out to be as expensive as polling anyway, so it never went anywhere.

I'll leave the flag, for API and info purposes, but I'll fix it to be > 0.

@headius headius added this to the JRuby 9.1.15.0 milestone Nov 9, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 9, 2017

Member

Yeah, this flag was all over the place. I also found the following logic, which seems to disable polling when there's one (or zero?) threads, so this isn't even internally consistent.

I'm removing the field, deprecating everything involving it, and modifying getPolling to use > 1 since that's what this was intended to indicate.

Member

headius commented Nov 9, 2017

Yeah, this flag was all over the place. I also found the following logic, which seems to disable polling when there's one (or zero?) threads, so this isn't even internally consistent.

I'm removing the field, deprecating everything involving it, and modifying getPolling to use > 1 since that's what this was intended to indicate.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 9, 2017

Member

Also worth noting that the access to the map is costly (slower thread init/teardown times), non-atomic (so it's only useful for an immediately-stale point-in-time snapshot) and the boolean is also non-atomically updated, so threads can step on each other.

Just a half-baked feature that never landed.

Member

headius commented Nov 9, 2017

Also worth noting that the access to the map is costly (slower thread init/teardown times), non-atomic (so it's only useful for an immediately-stale point-in-time snapshot) and the boolean is also non-atomically updated, so threads can step on each other.

Just a half-baked feature that never landed.

@headius headius closed this in 72412fd Nov 9, 2017

@nglorioso

This comment has been minimized.

Show comment
Hide comment
@nglorioso

nglorioso Nov 9, 2017

Contributor

Thanks for the fix!

Contributor

nglorioso commented Nov 9, 2017

Thanks for the fix!

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