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.

Sign up for GitHub

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

Jump to bottom

Catch invalid use of Server.RegisterService after Register.Serve #828

Merged
merged 1 commit into from
Jun 26, 2017
Merged

Catch invalid use of Server.RegisterService after Register.Serve #828

merged 1 commit into from
Jun 26, 2017
+5 −0

Conversation

petermattis
Copy link
Contributor

No description provided.

@menghanl
Copy link
Contributor

Did you encounter any problem caused by this?
By design, s.m should be read only after server.start, so this lock is not necessary here.

@petermattis
Copy link
Contributor Author

Yes, I did encounter a problem which motivated this. I have a scenario in which I wish to register a service on a server after initiating a connection to a server. If this isn't an allowed scenario, Server.register should disallow it.

@petermattis
Copy link
Contributor Author

Ah, I missed the comment above Server.RegisterService. Do you want me to add the protection against calling RegisterService after Server.Serve has been called or just drop this PR?

@menghanl
Copy link
Contributor

menghanl commented Aug 10, 2016 •
edited
Loading

I don't recall any label in Server indicating whether the server has started. Not sure how easy it is. Please add the protection if you find it easy to do.

@petermattis petermattis force-pushed the pmattis/fix-concurrent-map-read-and-write branch from bdcb981 to e1c6578 Compare August 11, 2016 00:06
@petermattis petermattis changed the title Lock read access to Server.m Catch invalid use of Server.RegisterService after Register.Serve Aug 11, 2016
@petermattis petermattis force-pushed the pmattis/fix-concurrent-map-read-and-write branch from e1c6578 to 945b39b Compare August 11, 2016 00:07
@petermattis
Copy link
Contributor Author

@menghanl This is what I was thinking of. Currently difficult to test this as there is no way to temporarily swap out the grpclog.logger for a single test.

@menghanl menghanl self-assigned this Jun 9, 2017
@menghanl
Copy link
Contributor

Sorry for the delay.
If you don't mind, can you resolve the conflict and let's get this in?

@petermattis petermattis force-pushed the pmattis/fix-concurrent-map-read-and-write branch from 945b39b to af72c65 Compare June 24, 2017 18:17
@petermattis
Copy link
Contributor Author

@menghanl Rebased.

menghanl
menghanl approved these changes Jun 26, 2017
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. LGTM.

@menghanl menghanl merged commit 57ff328 into grpc:master Jun 26, 2017
@menghanl menghanl added 1.5 Type: Behavior Change Behavior changes not categorized as bugs labels Jun 26, 2017
@menghanl menghanl mentioned this pull request Jul 13, 2017
Closed
@petermattis petermattis deleted the pmattis/fix-concurrent-map-read-and-write branch July 15, 2017 00:19
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@menghanl menghanl

Assignees

@menghanl menghanl

Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@petermattis @menghanl