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

Initialize the loops waitGroup when initializing the server #184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pooya
Copy link
Contributor

@pooya pooya commented Sep 8, 2017

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 shayan@arista.com

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 <shayan@arista.com>
@googlebot
Copy link
Collaborator

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

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

@hanwen
Copy link
Owner

hanwen commented Sep 11, 2017

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.

@pooya
Copy link
Contributor Author

pooya commented Sep 11, 2017

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.

@hanwen
Copy link
Owner

hanwen commented Sep 12, 2017

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?

@pooya
Copy link
Contributor Author

pooya commented Sep 12, 2017

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().

@hanwen
Copy link
Owner

hanwen commented Sep 12, 2017

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.

@pooya
Copy link
Contributor Author

pooya commented Sep 12, 2017

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.

@hanwen
Copy link
Owner

hanwen commented Sep 12, 2017

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

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.

None yet

3 participants