fuse-rs
fuse-rs copied to clipboard
zargony
Replace thread-scoped with crossbeam for spawn_mount
Hello! I took a stab at a 1-1 replacement for thread-scoped, in order to avoid the unsafe declaration. Works well for me so far.
Crossbeam requires the session to have a shorter lifetime than the scope, so I introduced another lifetime param. I could also remove the Debug implementation for BackgroundSession, and added code to join the background thread in the Drop implementation.
I wrote a test as well, but then I realized that it won't run in Travis, so I converted it to an example :)
It seems reasonable, but beware that this makes crossbeam a public dependency due to that exposed Scope. (i.e. updating to a new semver of crossbeam would be a breaking change.) Plus, the user has to call crossbeam::scope to get a Scope in the first place.
@cuviper Yes, a public dependency is unfortunate. I hadn't really thought about it that way. But tying the thread lifetime to such a scope is the way that crossbeam gets around the unsafety, so I don't really think you can hide it.
One way to minimize the dependency is to re-export those necessary items (scope and Scope), so fuse users don't have to directly depend on crossbeam themselves. Or they can use a different version of crossbeam if they like, as long as they still use the right scopes for fuse. It's still a breaking change to bump crossbeam though.
Neat. It wasn't clear to me that that was possible with re-exporting. I just tested it, it worked like a charm. I can change the PR to do that if you would like me to. If you dislike the extra dependency in general, I guess the alternative is to export unmount or to build some other mechanism to stop the session programmatically, without relying on threads explicitly.
I think re-exports are a good idea, but I'll leave it to @zargony to judge the public dependency. This will also need a semver bump for the change to existing API.
Sounds good. I re-exported scope and Scope like you suggested. I'm a bit unsure what happens with the public BackgroundSession struct, which has a crossbeam ScopedJoinHandle member. Will that be completely encapsulated from clients of fuse? When I try it, it doesn't seem like clients are required to bring the ScopedJoinHandle type into scope in order to use BackgroundSession.
I'm a bit unsure what happens with the public BackgroundSession struct, which has a crossbeam ScopedJoinHandle member. Will that be completely encapsulated from clients of fuse?
That will be private, yes. The only thing to check is its effect on auto-traits, namely Send and Sync.
@cuviper Thanks. If I interpret you correctly, ScopedJoinHandle (as well as Option) implements both Send and Sync, so BackgroundSession shouldn't lose those traits.
Oh wait, I didn't realize that the fields of BackgroundSession are pub. In that case it does become another part of the public dependency.
But I question whether the field should be pub at all -- the only thing you can do with the current guard is call thread(), which we could provide indirectly. Calling join() would require moving the field out, which isn't allowed with Drop. I suppose you could mem::swap a different guard in there, but that possibility seems even more reason that this shouldn't be pub.
I actually overlooked the pub part as well. I agree that it doesn't make much sense. I'll change both fields to be private.
I never worked with crossbeam before, but I see the advantage here. I generally like this changes, however I actually think that rust-fuse shouldn't deal with threads at all and should leave that to downstream crates. BackgroundSession is kind of a workaround to be able to mount a fuse session in the background. Imo BackgroundSession should be removed completely and Session should gain a future based interface that'll allow users to run it concurrently with other code.
I started to do some work towards this in #125 . The question would be: since async support still requires nightly Rust for a few more months, is it possible (i.e. worth the effort) to have an optional async feature so that rust-fuse keeps working with stable Rust, or should we maintain a 0.3 branch with changes like this?
