Skip to content

/ go

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

Jump to bottom

net/http: finishRequest() aborts ongoing background reads #17559

Closed
dhananjay92 opened this issue Oct 23, 2016 · 4 comments
Closed

net/http: finishRequest() aborts ongoing background reads #17559

dhananjay92 opened this issue Oct 23, 2016 · 4 comments
Assignees
@bradfitz
Labels
FrozenDueToAge NeedsInvestigation
Milestone
Go1.8

Comments

@dhananjay92
Copy link
Contributor

@dhananjay92 dhananjay92 commented Oct 23, 2016

[ continued from Issue #17547 ]

TL;DR: abortPendingRead() is called from finishRequest(), causing it to abort ongoing reads. This causes deadline_exceeded errors. finishRequest() should wait for reads to finish in normal case.

Consider following sequence of events from http.conn.serve():

  1. startBackgroundRead() starts backgroundRead() in another goroutine.
  2. Assume that backgroundRead() from above step does not finish until after the next step. (Simulated by adding sleep)
  3. finishRequest() called, which calls abortPendingRead(). abortPendingRead() then proceeds to abort ongoing background reads by setting the read deadline to aLongTimeAgo. This causes the ongoing reads to report deadline_exceeded.

/cc @bradfitz @dsnet @mdempsky
@bradfitz bradfitz self-assigned this Oct 23, 2016
@bradfitz bradfitz added this to the Go1.8 milestone Oct 23, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 23, 2016

I haven't looked into this yet. To clarify, are you saying this is a regression since faf882d (golang.org/cl/31173)?
@dhananjay92
Copy link
Contributor Author

@dhananjay92 dhananjay92 commented Oct 23, 2016 •
edited

Regression: Yes. I think so.
I have a CL that should fix this. Sending in a bit.
dhananjay92 added a commit to dhananjay92/go that referenced this issue Oct 23, 2016
@dhananjay92
net/http: Wait for background-reads to finish.
e7297d5 
http.response.finishRequest() calls abortPendingRead()
which then aborts the background reads. It should not
abort background reads, but wait for them to finish.

This CL,
    1.  changes abortPendingRead() to finishPendingRead()
        with option for aborting. Now for normal requests,
        it won't abort connections.
    2.  Sets the Read/Write deadlines for both TLS and TCP
        connections. Previously, it only set deadlines for
        TLS leading to deadlocks. These deadlocks went unnoticed
        because with abortPendingRead() everything was getting
        aborted all the time.
    3.  Ensures sensible default timeout values.

Fixes golang#17559.
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 23, 2016

CL https://golang.org/cl/31755 mentions this issue.
@dhananjay92 dhananjay92 mentioned this issue Oct 24, 2016
Closed
@dhananjay92
Copy link
Contributor Author

@dhananjay92 dhananjay92 commented Oct 24, 2016 •
edited

From offline discussion with Brad & Joe: It's okay for finishRequest() to abort backgroundRead() every time. The imeplementation of net.Conn should respect this semantics.

This is WAI. So closing this.
@brutella brutella mentioned this issue Feb 27, 2017
Closed
@golang golang locked and limited conversation to collaborators Oct 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bradfitz bradfitz

Labels
FrozenDueToAge NeedsInvestigation
Projects
None yet
Milestone
Go1.8
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@bradfitz @dhananjay92 @gopherbot
You can’t perform that action at this time.