windows-threading: Implement scope for Pool
#3696
Conversation
ChrisDenton
changed the title
Send for windows_threading::Poolscope for Pool
|
Ok, looking at this more I do think we need a scope API to ensure this is robust so I've changed this PR to implement that, While investigating this (and part of what drove this conclusion) I found that the service threading sample is not technically sound. Specifically these lines: windows-rs/crates/samples/services/thread/src/main.rs Lines 16 to 20 in 897ea82
I think this requires changes in the service API to support either a reference counted |
ChrisDenton
force-pushed
the
not-send
branch
2 times, most recently
from
c627ff6 to
191d1b1
Compare
| let service_original = Arc::new(RwLock::new(Service::new())); | ||
| let service = Arc::clone(&service_original); | ||
| service_original |
There was a problem hiding this comment.
This seems overly complicated. The original Service::run terminated the process upon completion so that the run method was -> ! and the lifetime was much simpler. This was changed in #3662 to make it possible to test Service but perhaps that was a mistake.
There was a problem hiding this comment.
Or the Service::run needs a similar scope pattern?
There was a problem hiding this comment.
I wonder if it might be better (and simpler) to separate Service into two: one that constructs the service and another that accesses it afterward. The latter could then be freely moved, cloned, etc because it's just accessing global state (via appropriate locks).
Using the scoped pattern may be another option. I'm still not sure if that'd be more or less ergonomic though. It does need to be able to call the closure multiple times which complicates things. It might help if run consumed self rather than taking &mut self. I'd want to play about with this a bit more to see.
|
I definitely found a friction between Rust's lifetime model and the various restrictions in the threadpool APIs. I much prefer the addition of the |
|
I played around with this today - I don't love how this impacts |
|
I've been thinking a lot about the Service API and yeah, I don't love the workaround in this PR. It does need changes to the Service API itself but I'd be happy to leave that to follow up work. |
2 participants
This is a fix #3695 that adds a
scopeAPI.