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

fall back gracefully when max thread IDs are reached #62

Open
hawkw opened this issue Aug 2, 2021 · 2 comments
Open

fall back gracefully when max thread IDs are reached #62

hawkw opened this issue Aug 2, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@hawkw
Copy link
Owner

hawkw commented Aug 2, 2021

When the thread ID limit defined by the Config is reached, we currently panic. This isn't great.

As I said in tokio-rs/tracing#1485:

I do think sharded-slab could handle this better. We could potentially reserve the last thread ID (in this case, 4096) for a special shard that allows concurrent mutation by multiple threads, with a mutex, and use that as a fallback when the thread ID cap is reached. That way, rather than panicking, we'd degrade to contending a mutex for threads over the max, which might be a better solution. However, that would be a bit of work to implement...but I'll open a ticket for it upstream.

@hawkw hawkw added the enhancement New feature or request label Aug 2, 2021
@danielhenrymantilla
Copy link
Contributor

I'm guessing this is the cause of a panic at the following line:

let shard = self.shards[idx].load(Relaxed).unwrap_or_else(|| {
?

If anything, replacing that [idx] with .get(idx).expect("too many threads") would already be quite helpful when debugging 🙂

@hawkw
Copy link
Owner Author

hawkw commented Oct 9, 2021

I'm guessing this is the cause of a panic at the following line:

let shard = self.shards[idx].load(Relaxed).unwrap_or_else(|| {
?

If anything, replacing that [idx] with .get(idx).expect("too many threads") would already be quite helpful when debugging 🙂

Yeah, that's a good point; we should at least make the panic message a bit more helpful in that case (and perhaps include what the configured thread count is).

I'd happily merge a PR that makes this change, if you're interested in opening one? Otherwise, I'll try to improve it when I get the chance.

danielhenrymantilla added a commit to danielhenrymantilla/sharded-slab that referenced this issue Oct 11, 2021
danielhenrymantilla added a commit to danielhenrymantilla/sharded-slab that referenced this issue Oct 11, 2021
hawkw added a commit that referenced this issue Oct 11, 2021
* Emit a nicer panic when thread count overflows `MAX_SHARDS`

Tackles #62 (comment)

* Replace `if cond { panic` with `assert!`

TODO: negate the condition in the assertion

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

* Negate the condition in the assertion

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants