fix sync issues when using standalone http receivers #541
+12
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
n3wscott
force-pushed
the
fix-standalone-write-errors
branch
4 times, most recently
from
b6f80bf to
96ea4d8
Compare
Signed-off-by: Scott Nichols <snichols@vmware.com>
n3wscott
force-pushed
the
fix-standalone-write-errors
branch
from
96ea4d8 to
d185f6a
Compare
vaikas
reviewed
Jun 25, 2020
| go func() { | ||
| r.p.ServeHTTP(rw, req) | ||
| wg.Done() |
There was a problem hiding this comment.
I admittedly don't have the full context just saw this floating by so apologies for likely noise :)
Can't we just call this synchronously instead of in a func? Am I missing something? Perhaps worthy of a comment so it's easier to reason about, but it's probably just me :)
There was a problem hiding this comment.
There are internal details of the SDK that require these two parts to work in parallel based on how the bindings work is documented, so yeah the server needs to go and it will block until the receive call is finished which is the second block of blocking call, but the bug was the function can't exit until ServeHTTP has finished, which is blocked on the callback...
yikes
slinkydeveloper
approved these changes
Jun 26, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #537
Thanks for the repo-repo @maschmid it helped a lot to track this down.
The root cause was the function that delegated to the ServeHTTP was exiting too soon and closing the http connection, so the bindings message to event function was attempting to read the large body out of a closed http connection, throwing an error, attempting to return a 500, but unable to do that because the ... connection was already closed.
Adding a sync group to the standalone ServeHTTP method and also using sync.WaitGroup in the normal ServeHTTP to be consistent (I know, a little overkill, but I think it might be better than just a done channel. It gives options for batch mode in the near future.)