-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: client prompt return on context cancellation #1729
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Umegbewe Nwebedu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some questions, not sure if this will work. My understanding was that we have to manually chunk reads from the resp.Body.
}() | ||
|
||
select { | ||
case <-ctx.Done(): | ||
resp.Body.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that close will somehow cancel _, err := buf.ReadFrom(resp.Body)
. Is it true? How?
Can we have a unit tests that perhaps shows this? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, calling resp.Body.Close()
will close the underlying connection, which unblocks any pending read and returns an error.
see unit test, if okay i will add this to client_test.go
func TestDoContextCancellation(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("partial"))
if f, ok := w.(http.Flusher); ok {
f.Flush()
}
time.Sleep(500 * time.Millisecond) // simulate delay
w.Write([]byte(" remaining"))
}))
defer ts.Close()
client, err := NewClient(Config{
Address: ts.URL,
})
if err != nil {
t.Fatalf("failed to create client: %v", err)
}
req, err := http.NewRequest("GET", ts.URL, nil)
if err != nil {
t.Fatalf("failed to create request: %v", err)
}
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
defer cancel()
start := time.Now()
resp, body, err := client.Do(ctx, req)
elapsed := time.Since(start)
if err != context.DeadlineExceeded {
t.Errorf("expected error %v, got: %v", context.DeadlineExceeded, err)
}
if body != nil {
t.Errorf("expected no body due to cancellation, got: %q", string(body))
}
if elapsed > 200*time.Millisecond {
t.Errorf("Do did not return promptly on cancellation: took %v", elapsed)
}
if resp != nil && resp.Body != nil {
resp.Body.Close()
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwplotka could you take a look 👀
This fixes the issue where the client blocks for the entire duration of the response stream, even when the provided context had already been canceled or times out
Relevant issue #1698