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.

Sign up for GitHub

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

set the connection close header if we have a body to read #116

Merged
merged 6 commits into from
Dec 13, 2018
Merged

set the connection close header if we have a body to read #116

merged 6 commits into from
Dec 13, 2018

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Sep 11, 2018 •
edited
Loading

We can't stream a response while reading a body unless we've configured the connection to close after we're done sending a response.

This patch works around that issue by doing exactly that.

Fixes: ipfs/kubo#5168 (sort of)

@ghost ghost assigned Stebalien Sep 11, 2018
@ghost ghost added the status/in-progress In progress label Sep 11, 2018
@Stebalien
Copy link
Member Author

This should probably have tests.

@Stebalien Stebalien mentioned this pull request Sep 11, 2018
Closed
@hsanjuan
Copy link
Contributor

I'm wondering how this is different from disabling keep alives on the server?

I haven't read too closely, but SetKeepAlivesEnabled(false) seems used when managing the connection pool in the http server, so maybe manually telling clients to cut it out leaves some undesired effects?

Also, this is equivalent to asking the client to close the connection but you will need the client to comply (hopefully). Wouldn't it be better to close server-side?

@Stebalien
Copy link
Member Author

See: ipfs/kubo#5168 (comment)

Basically, if we set SetKeepAlivesEnabled(false), we'll cut all connections. We don't want to do that for the gateway and, ideally, don't want do do that for 90% of what we do anyways. With this patch, we'll ask the client to close the connection if, and only if, they've sent us the body.

Ideally, we'd only do this if they've sent us a body and we decide to send back a response before reading the rest of the request. However, harder to implement.

@Stebalien
Copy link
Member Author

@keks thoughts?

@keks
Copy link
Contributor

keks commented Sep 19, 2018

Wow this is a tricky one.

Ideally, we'd only do this if they've sent us a body and we decide to send back a response before reading the rest of the request.

We could check this in the http response emitter preamble function. However, I think in there we don't know whether we read the entire body yet. Instead of reading the body directly, we could make a wrapper that just calls req.Body.Read and sets a flag when this returns io.EOF. We could then check that flag in the preamble. I'm just afraid that we might forget reading from the wrapper somewhere and instead read directly from req.Body... But we can overwrite that, right?

@Stebalien
Copy link
Member Author

But we can overwrite that, right?

I think so... Feel free to take this over and try it.

@Stebalien Stebalien assigned keks and unassigned Stebalien Sep 20, 2018
@Stebalien
Copy link
Member Author

@keks does this look good to go as a stop-gap so we can fix the current issue?

@hsanjuan hsanjuan mentioned this pull request Sep 26, 2018
Closed
14 tasks
@keks
Copy link
Contributor

keks commented Sep 28, 2018

@Stebalien sorry I had an exam on Wednesday. Looking into it right now.

@Stebalien
Copy link
Member Author

Np.

@Stebalien Stebalien requested a review from keks as a code owner October 1, 2018 14:45
@keks
Copy link
Contributor

keks commented Oct 1, 2018

I'm still thinking whether we can come up with a good unit test for this...

Stebalien
Stebalien commented Oct 1, 2018
View reviewed changes
Copy link
Member Author

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Instead of writing the error to the channel, we may want to consider simply closing it on EOF (unless we just want to assume that we can't have multiple errors).

http/responseemitter.go Outdated
// FIXME: https://github.com/golang/go/issues/15527
if re.bodyErrChan != nil {
select {
case <-re.bodyErrChan:
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we can get transient errors (e.g., read timeouts). We probably do just want to check for an EOF and assume that other errors mean there may be more data. In all likelihood, any non-EOF error means the connection is in trouble anyways.

http/handler.go Outdated
// FIXME: https://github.com/golang/go/issues/15527
var bodyErrChan chan error
if r.Body != http.NoBody {
bodyErrChan = make(chan error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Needs a buffer.

@keks
Copy link
Contributor

keks commented Oct 2, 2018

Weird. I added a check that the handler actually sets the "Connection: close" header, but the client doesn't see it. However, when I look at the test coverage, the line where the header is set is green, so the handler really should set it.

@keks
Copy link
Contributor

keks commented Oct 3, 2018

I don't get why I can't comment inline, but whatever....

You may need to pre-close this channel in the else case.

In the else case the channel is nil (it's only make'd in the if branch), so the bodyEOFChan in the response emitter is nil in that case. And in the preamble I only try to read from it if it's not nil, so we'll never try to read the channel if we didn't catch the if branch here. Makes sense?

@lidel lidel mentioned this pull request Oct 3, 2018
Open
4 tasks
@keks
Copy link
Contributor

keks commented Oct 4, 2018

@Stebalien have you seen my previous comment?

@Stebalien
Copy link
Member Author

I have, I just have no idea what's going on either.

@keks
Copy link
Contributor

keks commented Oct 4, 2018

Hm? I commented on your review remark, and I think your request for change is not necessary. Do you agree? If not, why?

@Stebalien
Copy link
Member Author

Hm? I commented on your review remark, and I think your request for change is not necessary. Do you agree? If not, why?

Ah, yes, you were right. That looks correct.

@keks keks requested review from magik6k and removed request for keks and magik6k October 5, 2018 07:00
@ghost ghost assigned Stebalien Nov 8, 2018
@Stebalien
Copy link
Member Author

@keks any chance we can get this moving? The tests aren't currently passing but we need to get some fix for this in.

@eingenito eingenito mentioned this pull request Nov 10, 2018
Open
@Stebalien
Copy link
Member Author

Ok, I've finally found the issue (go swallows the connection close header and sticks it in the response object).

@Stebalien
Copy link
Member Author

@keks wrote the final version so I'll give it a ✔️ review.

@Stebalien Stebalien merged commit 94295ed into master Dec 13, 2018
@ghost ghost removed the status/in-progress In progress label Dec 13, 2018
@Stebalien Stebalien deleted the fix/go-ipfs-5168 branch December 27, 2018 05:53
@Stebalien Stebalien mentioned this pull request Jun 4, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keks keks keks left review comments

Assignees

@keks keks

@Stebalien Stebalien

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error while files.add over HTTP API (http: invalid Read on closed Body)
3 participants
@Stebalien @hsanjuan @keks