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

Use a new buffer to avoid race conditions #112

Merged
merged 3 commits into from
Sep 22, 2017
Merged

Conversation

gabsn
Copy link

@gabsn gabsn commented Sep 8, 2017

Here we do 2 things:

  1. We use a new buffer before sending the encoder through the network to avoid race conditions (the new buffer uses the same underlying data, so there is no performance overhead)
  2. We stop fully reading the response, to avoid those logs:
Unsolicited response received on idle HTTP channel starting with "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain; charset=utf-8\r\nConnection: close\r\n\r\n400 Bad Request"; err=<nil>

Actually, this trick is according to people able to improve the client performances by reusing persistent connections, but it's not documented in the official golang documentation. Moreover, we had those unwanted logs by doing this. So my point here is we should not use this unsafe trick in such an important part of the trace and rather consider using a grpc library like this one that will natively use HTTP2.

I also considered using a pool of buffers to avoid reallocating memory, but it was still leading to race conditions. Indeed, once the function client.Do(req) has been called, the persitentConn.writeLoop goroutine will be spawned with a reference on the buffer, and will try to read it later, even when we will put the buffer back to the pool. That's why to be safe, it's preferable to create a new buffer each time we call client.Do (again, we don't really create a new buffer, since we reuse the underlying slice of bytes).

@gabsn gabsn changed the title Use buffer pool to avoid race conditions Use a buffer pool to avoid race conditions Sep 8, 2017
@gabsn gabsn changed the title Use a buffer pool to avoid race conditions Use a new buffer to avoid race conditions Sep 12, 2017
@gabsn gabsn requested a review from palazzem September 12, 2017 15:25
@@ -59,6 +65,16 @@ func (e *msgpackEncoder) ContentType() string {
return e.contentType
}

func (e *msgpackEncoder) Buffer() *bytes.Buffer {
return e.buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

probably we can add a lock to be sure it's thread-safe

@palazzem palazzem added this to the 0.5.1 milestone Sep 22, 2017
@palazzem palazzem added core enhancement quick change/addition that does not need full team approval labels Sep 22, 2017
@palazzem palazzem merged commit 9234800 into master Sep 22, 2017
@palazzem palazzem deleted the gabin/buffer-pool branch September 22, 2017 15:34
@gabsn gabsn mentioned this pull request Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement quick change/addition that does not need full team approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants