-
Notifications
You must be signed in to change notification settings - Fork 344
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
base: master
Are you sure you want to change the base?
Conversation
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]>
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.
|
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. |
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. |
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? |
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(). |
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. |
Yes. But changing the API in that way would be backwards-incompatible I assume. |
Let me see what kind of problems changing the API would create. |
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]