go-fuse icon indicating copy to clipboard operation
go-fuse copied to clipboard

Initialize the loops waitGroup when initializing the server

Open pooya opened this issue 6 years ago • 8 comments

Otherwise, the Serve() and Unmount() calls will race calling loops.Add(1) and loops.Wait() and the outcome is undefined.

Signed-off-by: Shayan Pooya [email protected]

pooya avatar Sep 08 '17 19:09 pooya

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot avatar Sep 08 '17 19:09 googlebot

your two pull requests seem conflicting: if you are always calling WaitMount, then you would always call Unmount after WaitMount. Since WaitMount waits for Serve to process the first request, there is no race.

hanwen avatar Sep 11 '17 21:09 hanwen

The main purpose of this patch is to have a way to correctly schedule a umount to be called after a hard deadline; to unmount and kill any run-away servers. If there is something wrong with the fuse server, the WaitMount() itself might hang. So I don't think it is the best idea to start such a timer after WaitMount() returns.

pooya avatar Sep 11 '17 21:09 pooya

can you specify more exactly what "something wrong" means? If your server is broken and in an unknown state, then is there any sense in worrying about deadlines?

hanwen avatar Sep 12 '17 08:09 hanwen

This is not an issue for the production use-case where the server's state is unknown. We are trying to run a server in a test and we have a timeout to kill the server (and fail the test) if it runs for too long. Using golang's -race option correctly reports this race between calling Serve() and Unmount().

pooya avatar Sep 12 '17 16:09 pooya

I think the real problem is that the API is designed in the wrong way.

The Serve loop should be started from NewServer. Possibly, it should even do WaitMount implicitly.

Then there should be a Server.Wait() which waits until the serve loop exits.

hanwen avatar Sep 12 '17 19:09 hanwen

Yes. But changing the API in that way would be backwards-incompatible I assume. Of course, up to you to decide whether to incorporate this change or not, not a big issue.

pooya avatar Sep 12 '17 21:09 pooya

Let me see what kind of problems changing the API would create.

hanwen avatar Sep 12 '17 22:09 hanwen