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

fix racy access of err variable #575

Merged

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Mar 16, 2018

Without this fix, the handleSend function is closing over err from the outer function. This trips up the Go race detector because handleSend gets invoked in a goroutine and there is no synchronization between that goroutine and a subsequent write to err after a call to stream.Header.

This appears to be an accidental closure -- handleSend doesn't actually need to share the err variable so can instead just create its own err local variable. Which clears it up.

@jhump
Copy link
Contributor Author

jhump commented Mar 16, 2018

Ping @achew22. This is causing failing tests for some of our stuff because every bidi stream invocation tickles the race detector. We've temporarily forked grpc-gateway. This seems like a non-controversial fix, so we were hoping to not have to maintain a fork for very long.

@achew22
Copy link
Collaborator

achew22 commented Mar 29, 2018

Can you help me understand why changing the template doesn't change any of the generated code in the examples section? Could you run make examples and include any changes that are in there?

@jhump
Copy link
Contributor Author

jhump commented Mar 29, 2018

@achew22: I didn't even realize that there was an examples folder.

I ran make generate, but it produced a ton of differences. I am guessing I have a different version of swagger-codegen than was used to produce the existing files. (I actually just installed it on OS X: brew install swagger-codegen. So maybe the existing swagger-generated files need to be freshened?)

I only git add'ed the ones relevant to my change.

@jhump
Copy link
Contributor Author

jhump commented Apr 11, 2018

ping: @achew22, I think this branch is ready to go.

@achew22
Copy link
Collaborator

achew22 commented Apr 15, 2018

Could you rebase this change? I've fixed the test that was not running that I was expecting to fail in here.

Thanks!

@jhump jhump force-pushed the jh/fix-race-condition-in-bidi-streams branch from 2505a2c to fd60141 Compare April 16, 2018 14:38
@jhump
Copy link
Contributor Author

jhump commented Apr 16, 2018

Could you rebase this change?

@achew, done

@achew22 achew22 merged commit 88ac76f into grpc-ecosystem:master Apr 16, 2018
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants