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

Jump to bottom

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

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

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

JayNakrani opened this issue Oct 23, 2016 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.8

Comments

@JayNakrani
Copy link
Contributor

[ 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 bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 23, 2016
@bradfitz
Copy link
Contributor

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

@JayNakrani
Copy link
Contributor Author

JayNakrani commented Oct 23, 2016 •
edited

Regression: Yes. I think so.
I have a CL that should fix this. Sending in a bit.

JayNakrani added a commit to JayNakrani/go that referenced this issue Oct 23, 2016
@JayNakrani
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

CL https://golang.org/cl/31755 mentions this issue.

@JayNakrani JayNakrani mentioned this issue Oct 24, 2016
Closed
@JayNakrani
Copy link
Contributor Author

JayNakrani 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
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
3 participants
@bradfitz @JayNakrani @gopherbot