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

Replace thread-scoped with crossbeam for spawn_mount #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atorstling
Copy link

@atorstling atorstling commented Dec 7, 2018

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 :)

@cuviper
Copy link
Contributor

cuviper commented Dec 7, 2018

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.

@atorstling
Copy link
Author

@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.

@cuviper
Copy link
Contributor

cuviper commented Dec 7, 2018

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.

@atorstling
Copy link
Author

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.

@cuviper
Copy link
Contributor

cuviper commented Dec 7, 2018

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.

@atorstling
Copy link
Author

atorstling commented Dec 7, 2018

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.

@cuviper
Copy link
Contributor

cuviper commented Dec 7, 2018

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.

@atorstling
Copy link
Author

@cuviper Thanks. If I interpret you correctly, ScopedJoinHandle (as well as Option) implements both Send and Sync, so BackgroundSession shouldn't lose those traits.

@cuviper
Copy link
Contributor

cuviper commented Dec 7, 2018

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.

@atorstling
Copy link
Author

atorstling commented Dec 7, 2018

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.

@zargony
Copy link
Owner

zargony commented Jun 19, 2019

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants