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

relay: disable keep alive conns, they interact poorly with suspension #365

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

skabbes
Copy link
Contributor

@skabbes skabbes commented Oct 10, 2017

I expect the DisableKeepAlives change to eliminate the EOF / conn reset errors. Without those errors we would never have found the re-read request body error, which is explicitly addressed by the new GetBody code.

A few justifications for the other changes:

  1. remove all http.Transport timeouts. These are generally to prevent "bad" upstream servers, but I think up should try to be as invisible as possible, including not adding additional http timeouts. We do not know how people will use Up. Maybe they want to use it for delayed Continue responses.
  2. remove specific GetBody code, this is already handled by http.NewRequest
  3. Tests show new behavior for disabling keep-alives

@skabbes
Copy link
Contributor Author

skabbes commented Oct 10, 2017

@tj @berzniz another promising change for the contentlength errors, TBH the DisableKeepAlives is probably the entire fix.

@skabbes
Copy link
Contributor Author

skabbes commented Oct 10, 2017

Also, the reason the GetBody caused the 'no content' bug previously, was because internally http.NewRequest was type switching on the io.Reader passed as the body to set the ContentLength. ioutil.NopReader was hiding that. So, content length appeared to be 0 for all the requests.

@tj tj merged commit 653103b into apex:master Oct 10, 2017
@berzniz
Copy link

berzniz commented Oct 10, 2017

Thanks @skabbes 7 @tj! I'm running the new version and will report if it fixed the issues.

@skabbes skabbes deleted the fix/content_length branch October 10, 2017 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants