Make RegionHandlers be Send + Sync by default#318
Conversation
Preserve the option to have non Send/Sync RegionHandlers for anyone who finds that useful. There is an API change in `install_region_handler` - but region handlers are not currently supported so this should not be an issue.
| impl<H, R> BaseInterpreter<H, R> | ||
| where | ||
| H: Handler, | ||
| R: RegionHandler + ?Sized, |
There was a problem hiding this comment.
Hm I don't think I've ever used this pattern myself where you constrain a generic to be a dyn Trait in an aliasing type like this - does this constrain R to being a single type (i.e. all region handlers in the map have to be of the same type), or does it mean R represents a trait object like before?
There was a problem hiding this comment.
Unfortunately looks like this is the former. The Box does need to contain some kind of trait object to push it into being a fat pointer with a vtable etc.
Overall, I don't think it's an unreasonable requirement for RegionHandlers to be Send+Sync. I wonder if we should just constrain all of them to be?
There was a problem hiding this comment.
That's odd... The attached shows it working for me at least. Mind copying this to tests and seeing if it's just something weird in my config? region_handlers.rs.txt (drop the .txt of course)
There was a problem hiding this comment.
Oh - on line 18 you mean Baz instead of Bar, plus you need a ?Sized constraint on line 5.
There was a problem hiding this comment.
Overall, I don't think it's an unreasonable requirement for RegionHandlers to be Send+Sync. I wonder if we should just constrain all of them to be?
I did wonder about this quite a bit... I think there's a counter-argument of two parts
- A micro-kernel system would (presumably) run
acpiin a separate process. In which case it might well be single-threaded. - If it is single-threaded, then it may be useful to use non-send types like
Rcin aRegionHandler, and since there's a low-cost way for us to enable that, it's worth us enabling it.
But as always I'm open to your thoughts!
2 participants
This fixes #301 in my preferred way (the Send interpreter remains called
Interpreter) but still allows for an easy non-send interpreter as well.NB: The function signature of
install_region_handlerchanges to require aBoxto be passed to it. This was the simplest way of avoiding a compiler error aboutSized. Plus it relieves that function of the responsibility to construct a Box 😉I've done a bunch of testing to make sure this works as expected, but there's no easy way to write "compile fail" tests except as doctests - and the need for a dummy
RegionHandlerjust makes it look very messy. So I assert to you that it works correctly!Ideally I'd like to remove the
unsafe implblocks forInterpreter, but there's more work to do first (see also #307). This PR addresses probably the most obvious unsafety of pretending that allRegionHandlers are Send + Sync.