THRIFT-5324: reset http client buffer after flush #2293
Conversation
|
I've just discovered the same bug today and fixed it in my fork :) Your commit includes a test, so I'll merge yours. |
lib/go/thrift/http_client.go
Outdated
| @@ -197,6 +197,10 @@ func (p *THttpClient) Flush(ctx context.Context) error { | |||
| // Close any previous response body to avoid leaking connections. | |||
| p.closeResponse() | |||
|
|
|||
| // Request might not have been fully read by http client. | |||
| // Reset so we don't send the remains on next call. | |||
| defer p.requestBuffer.Reset() | |||
i512
force-pushed
the
THRIFT-5324
branch
3 times, most recently
from
97a6725
to
1ad9d79
Compare
lib/go/thrift/http_client_test.go
Outdated
| } | ||
| trans, err := NewTHttpPostClient("http://" + addr.String()) | ||
| if err != nil { | ||
| t.Fatalf("Unable to connect to %s: %s", addr.String(), err) |
There was a problem hiding this comment.
This is nitpicking and also a problem already exists in the old test code, but using %s on errors is the same as using %v on errors except that %s handles it much more poorer when error is nil (https://play.golang.org/p/yHovIp3m37B). I'd rather just use %v with errors instead.
lib/go/thrift/http_client_test.go
Outdated
|
|
||
| _, err = trans.Write(write1) | ||
| if err != nil { | ||
| t.Fatalf("Failed to write to transport: %s", err.Error()) |
There was a problem hiding this comment.
Similar to the comment above, instead of using %s with err.Error(), just use %v with err.
lib/go/thrift/http_client_test.go
Outdated
| cancel() | ||
| err = trans.Flush(ctx) | ||
| if err == nil { | ||
| t.Fatalf("Expected flush error") |
There was a problem hiding this comment.
There's no formatting verbs so this can be t.Fatal instead of t.Fatalf.
lib/go/thrift/http_client_test.go
Outdated
| if !bytes.Equal(data, write2) { | ||
| t.Fatalf("Received unexpected data: %s", data) | ||
| } | ||
| trans.Close() |
There was a problem hiding this comment.
Please move this to right after you checked for error on Line 121 as defer trans.Close(). You have a lot of t.Fatalf that could exit the test early without closing the opened trans (this is test code so this is nitpicking).
lib/go/thrift/http_client_test.go
Outdated
|
|
||
| data = data[:n] | ||
| if !bytes.Equal(data, write2) { | ||
| t.Fatalf("Received unexpected data: %s", data) |
There was a problem hiding this comment.
My suggestions here are:
- Use
%qinstead of%s(sometimes when the test failed it's because you read zero data, with%sthe error message will show asReceived unexpected data:, while with%qit shows asReceived unexpected data: ""instead, which makes it much more obvious that the failure is due to empty string). - Also print out what's the expected data in the failure message (e.g.
t.Fatalf("Received data expected %q, got %q", write2, data))
Client: go THttpClient did not reset it's internal buffer when http client returned an error, leaving the whole or partially read message in the buffer. Now we reset the buffer in defer.
|
Merged, thanks for the PR! |
|
@NIHERASE I think the added test is flaky. If you scroll to the bottom of https://travis-ci.org/github/apache/thrift/jobs/749896929, there are a lot of data races caused by this added test, and also the test failed (looks like the failure is caused by the data race, but I'm not 100% sure), |
|
Saw the same test failure on one of the previous travis run on this PR (not on the latest version) as well: https://travis-ci.org/github/apache/thrift/jobs/749705982 |
|
OK I cannot reproduce this issue with go1.15.6 locally. Looking through travis log we were using go 1.10.8 to run that test, and I do can reproduce this issue with go1.10.8 almost 100% locally. My current understanding is that in the test we are relying on canceled context to make the http request fail: thrift/lib/go/thrift/http_client_test.go Line 129 in e872b35 This works fine in go1.15.6. In go1.10.8 although it did make the I think the big, underlying problem is that when making the http request inside thrift/lib/go/thrift/http_client.go Line 208 in e872b35 THttpClient thinks it owns the data and can call its Reset, while the http request also thinks it owns the data and it's the only one reading/writing from it.
I think the proper fix would be that we need to make a copy of the buffer before sending it to http request. It will hurt performance, though. @dcelasun what do you think? |
Client: go The fix in apache#2293 doesn't work for go1.10.8 due to the possibility of data races. This exposes a bigger, underlying issue regarding the ownership of the request buffer in THttpClient between THttpClient itself and the http request it creates. Instead of reset and reuse the same buffer, always give up the ownership of it and create a new buffer after each Flush call.
Client: go The fix in apache#2293 doesn't work for go1.10.8 due to the possibility of data races. This exposes a bigger, underlying issue regarding the ownership of the request buffer in THttpClient between THttpClient itself and the http request it creates. Instead of reset and reuse the same buffer, always give up the ownership of it and create a new buffer after each Flush call.
|
Replied on the other PR. |
Client: go The fix in #2293 doesn't work for go1.10.8 due to the possibility of data races. This exposes a bigger, underlying issue regarding the ownership of the request buffer in THttpClient between THttpClient itself and the http request it creates. Instead of reset and reuse the same buffer, always give up the ownership of it and create a new buffer after each Flush call.
Client: go
THttpClient did not reset it's internal buffer when http client returned
an error, leaving the whole or partially read message in the buffer.
Now we reset the buffer in defer.
Details: https://issues.apache.org/jira/browse/THRIFT-5324