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

Push always sends entire package twice with authenticated package sources #1501

Closed
zlrenner opened this Issue Oct 1, 2015 · 9 comments

Comments

Projects
None yet
6 participants
@zlrenner

zlrenner commented Oct 1, 2015

...or alternatively stated: push continues to send entire request even after server responds with an error (e.g. 4XX).

When pushing to an authenticated feed, push continues to send the entire request, even if the server has already seen the headers and determined that the request is invalid (e.g. due to authentication).

Something like "Expect: 100-Continue" and/or pre-authenticate should be used to prevent this.
Note: Having the server forcibly abort/reset the connection results in the client throwing "An existing connection was forcibly closed by the remote host".

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Oct 5, 2015

@johnataylor assigning over to you, this looks like another good optimization for push.

CC @maartenba

yishaigalatzer commented Oct 5, 2015

@johnataylor assigning over to you, this looks like another good optimization for push.

CC @maartenba

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Nov 23, 2015

@zlrenner it seems fine that the client tries to do both, the server just should avoid reading the body, isn't that what is going on?

yishaigalatzer commented Nov 23, 2015

@zlrenner it seems fine that the client tries to do both, the server just should avoid reading the body, isn't that what is going on?

@zarenner

This comment has been minimized.

Show comment
Hide comment
@zarenner

zarenner Nov 23, 2015

@yishaigalatzer The server cannot avoid reading the body, my understanding and testing shows that it must finish reading the body (even if it plans to discard it) because of HTTP message framing. Note that if the server forcibly closes the socket, the client throws, so that's not an option.

The two options I'm aware of are:

  1. Add support to the client (and server) for Expect: 100-continue, so the client doesn't actually send the payload until the server confirms the request headers are acceptable.
  2. Once the client performs a request with authentication, always perform future requests with authentication. Assuming this is implemented correctly, because we perform a GET before doing any PUTs, packages would only be put once. Solves the authenticated feeds problem only.

zarenner commented Nov 23, 2015

@yishaigalatzer The server cannot avoid reading the body, my understanding and testing shows that it must finish reading the body (even if it plans to discard it) because of HTTP message framing. Note that if the server forcibly closes the socket, the client throws, so that's not an option.

The two options I'm aware of are:

  1. Add support to the client (and server) for Expect: 100-continue, so the client doesn't actually send the payload until the server confirms the request headers are acceptable.
  2. Once the client performs a request with authentication, always perform future requests with authentication. Assuming this is implemented correctly, because we perform a GET before doing any PUTs, packages would only be put once. Solves the authenticated feeds problem only.
@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Nov 23, 2015

@zarenner agreed that we can't resolve it with a server only change.

We should carefully evaluate the suggestions 1/2, and see if there are other options.

Can you explain:

Solves the authenticated feeds problem only.

yishaigalatzer commented Nov 23, 2015

@zarenner agreed that we can't resolve it with a server only change.

We should carefully evaluate the suggestions 1/2, and see if there are other options.

Can you explain:

Solves the authenticated feeds problem only.

@zarenner

This comment has been minimized.

Show comment
Hide comment
@zarenner

zarenner Nov 23, 2015

2 is only a solution to the problem where authentication isn't tried until 401 response, and thus the package is sent twice. It is not a solution to the inefficiency ("problem" might be a bit debatable here) where a client sends invalid headers, e.g. invalid credentials or apikey, and the whole body is sent even though the server knew it was bad as soon as it inspected the headers.

zarenner commented Nov 23, 2015

2 is only a solution to the problem where authentication isn't tried until 401 response, and thus the package is sent twice. It is not a solution to the inefficiency ("problem" might be a bit debatable here) where a client sends invalid headers, e.g. invalid credentials or apikey, and the whole body is sent even though the server knew it was bad as soon as it inspected the headers.

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Nov 23, 2015

Ah, it can still abort it and we can still handle it

yishaigalatzer commented Nov 23, 2015

Ah, it can still abort it and we can still handle it

@zarenner

This comment has been minimized.

Show comment
Hide comment
@zarenner

zarenner Nov 23, 2015

My concern with aborting/forcibly closing the connection is that it's a hack, especially since the server can't actually return an error response to the client. From the client's perspective, the server just died.

I believe that Expect: 100-Continue is the correct solution, as it was designed for exactly this. What I don't completely know, however, is what happens when the server doesn't support it. http://blogs.msdn.com/b/fiddler/archive/2011/11/05/http-expect-continue-delays-transmitting-post-bodies-by-up-to-350-milliseconds.aspx indicates that .NET will wait 350ms before falling back on sending the full request, but it's something we'll have to check.

edit: Note that implementing Expect: 100-Continue doesn't preclude us from also implementing always-send-credentials-after-401ing to (e.g. on pushes of multiple packages) roughly half the number of requests.

zarenner commented Nov 23, 2015

My concern with aborting/forcibly closing the connection is that it's a hack, especially since the server can't actually return an error response to the client. From the client's perspective, the server just died.

I believe that Expect: 100-Continue is the correct solution, as it was designed for exactly this. What I don't completely know, however, is what happens when the server doesn't support it. http://blogs.msdn.com/b/fiddler/archive/2011/11/05/http-expect-continue-delays-transmitting-post-bodies-by-up-to-350-milliseconds.aspx indicates that .NET will wait 350ms before falling back on sending the full request, but it's something we'll have to check.

edit: Note that implementing Expect: 100-Continue doesn't preclude us from also implementing always-send-credentials-after-401ing to (e.g. on pushes of multiple packages) roughly half the number of requests.

@yishaigalatzer yishaigalatzer modified the milestones: 3.4 Beta, 3.4 RTM Feb 25, 2016

@yishaigalatzer yishaigalatzer modified the milestones: 3.4 RTM, 3.4 RTM - Triage, 3.5 Beta Mar 11, 2016

@spadapet

This comment has been minimized.

Show comment
Hide comment
@spadapet

spadapet Mar 30, 2016

After much investigation, I think I fully see what is going on.
Comments for the two suggestions:

  1. The client already sends Expect: 100-continue by default. It waits 350ms for the 100-continue before sending the whole body. So the server just needs to respond with failure or 100 before 350ms passes. No change is needed here. Some servers don't respond with 100 (like the one I tried, visualstudio.com) so the client will always wait 350ms.
  2. We already cache credentials after authenticating. I don't think a change is needed here. Doing a "pre-authenticate" is a hack and isn't needed as long as the 100-continue works.

The expect 100-continue wasn't working because we aren't sending the data in chunks. We're using a Content-Length so that the server knows how much data to expect. I debugged the .NET HttpWebRequest and see that the Content-Length will force the body to be sent. Only chunked data is allowed to be canceled.

So my only idea for a fix is to send chunked data, no Content-Length. Hopefully all servers can deal with that.

spadapet commented Mar 30, 2016

After much investigation, I think I fully see what is going on.
Comments for the two suggestions:

  1. The client already sends Expect: 100-continue by default. It waits 350ms for the 100-continue before sending the whole body. So the server just needs to respond with failure or 100 before 350ms passes. No change is needed here. Some servers don't respond with 100 (like the one I tried, visualstudio.com) so the client will always wait 350ms.
  2. We already cache credentials after authenticating. I don't think a change is needed here. Doing a "pre-authenticate" is a hack and isn't needed as long as the 100-continue works.

The expect 100-continue wasn't working because we aren't sending the data in chunks. We're using a Content-Length so that the server knows how much data to expect. I debugged the .NET HttpWebRequest and see that the Content-Length will force the body to be sent. Only chunked data is allowed to be canceled.

So my only idea for a fix is to send chunked data, no Content-Length. Hopefully all servers can deal with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment