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

Delayed 304 responses when using proxy #4298

Closed
bjowes opened this issue May 23, 2019 · 11 comments

Comments

4 participants
@bjowes
Copy link

commented May 23, 2019

Current behaviour:

When a page loads resources, and the resources have been loaded before, the web server returns a 304 response. When running Cypress without proxy, this works fine. When running Cypress with proxy, there is a 5 second delay before the response reaches the browser.

Desired behavior:

There should be no additional delay on 304 responses with a proxy.

Steps to reproduce: (app code and test code)

I have created a minimal demo for the issue. Please see:
https://github.com/bjowes/delayed-response-demo

The README contains instruction on using the demo.

In this simplistic case, the tests still pass. In my real scenario I am testing an Angular app with many more resources. The tests fail nearly every time due to timeout caused by delayed 304 responses.

Versions

Tested with Cypress 3.3.1 on Windows 10 and OS X 10.14.4.
Browser: Chrome v74.
Also tried in headless mode with Electron 61.
I haven't double checked but I believe the behaviour has been present at least since Cypress 3.1.0.

Comment

Since the proxy I use is made by me (and based on the node-http-mitm-proxy library), I realise that the error may lie in the proxy itself. However, I have verified that using Chrome with my proxy but without Cypress works just fine.

The only thing the node-http-mitm-proxy does to the traffic is setting transfer-encoding to chunked and removing the content-length header (if any). I have observed that if I remove this in the case of 304 response (which has no content) it works fine. Hence I presume there is some issue related to chunked encoding consisting of only an empty chunk.

Personally I can live with my workaround but I believe that it is not an unusual approach by proxies to always set transfer-encoding on responses to chunked, especially those that allow modification of the content.

@jennifer-shehane

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Hey @bjowes, we'll take a look.

Could you provide your current workaround? It may be helpful to others in this situation at the moment. Thanks!

@bjowes

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Sure thing @jennifer-shehane !
Workaround version of node-http-mitm-proxy: https://github.com/bjowes/node-http-mitm-proxy
Version 1.0.2 of cypress-ntlm-auth plugin uses this workaround, will be released during the day.

@bjowes

This comment has been minimized.

Copy link
Author

commented May 24, 2019

Note that the workaround is not implemented exactly as I originally described. The published workaround checks if there are any response filters registered in the proxy (response filters are used to modify the response content). If there aren't (which there won't be in case of cypress-ntlm-auth) the transfer encoding is left as it was in the response from the server.

@brian-mann

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@flotwig what are your initial thoughts about this issue as well as #4313 (comment)?

@flotwig

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@brian-mann For this issue, probably retrying under the hood when we shouldn't - there's an exact 5 second delay. For #4313, if it's not related to this, not sure.

@flotwig

This comment has been minimized.

Copy link
Member

commented May 30, 2019

The 5 seconds thing being from 3.3.0 turned out to be a red herring, this issue is present at least as early as Cypress 3.2.0.

I recorded a few scenarios, all in Cypress 3.2.0:

Trying to see if I can localize the issue to a particular part of the proxy layer now.

@bjowes

This comment has been minimized.

Copy link
Author

commented May 30, 2019

Good progress @flotwig! I suspected that this issues was present also in earlier releases.

Did you consider the possibility that this may not be a proxy issue - maybe the same delay would occur with cache and chunked even without the proxy. In my case, the proxy causes chunked encoding so those go hand in hand, but it could just as well occur without a proxy. Just a hunch 😎

@flotwig

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Yeah, you're probably right in that it's not specific to proxies.

I recorded the conversations between the browser <-> Cypress <-> the mitm proxy:

Conversation between the browser and Cypress:

image

Conversation between Cypress and the chunked proxy:

image

Conversation between the chunked proxy and the server:

image

I think that node-http-mitm-proxy doesn't actually send valid Transfer-Encoding: chunked replies. The correct way to indicate a 0-length body with chunked encoding is to send an empty chunk before ending:

0\r\n
\r\n

...but node-http-mitm-proxy doesn't send anything at all, just a blank body. Still, Chrome can handle this without needing to wait 5 seconds, so Cypress should be able to too.

EDIT: This Node.js test seems to suggest that this is expected: https://github.com/nodejs/node/blob/8b4af64f50c5e41ce0155716f294c24ccdecad03/test/parallel/test-http-chunked-304.js#L58-L63

EDIT 2: Relevant comment: https://github.com/nodejs/node/blob/00ba75ed5ef2132ccbe043b3993bd90e7362966b/lib/_http_outgoing.js#L319-L321

@bjowes

This comment has been minimized.

Copy link
Author

commented May 30, 2019

Yeah, found that in RFC 7230 section 3.3.3 point 1. I think the RFC is a bit too permissive - you can put any header you like on a 304 response, but you can’t send a body. That is a good recipe to confuse developers... It would probably have been easier if certain headers were simply not allowed in 304 responses (and similar non body responses)

But as it stands, I assume cypress needs to ignore those headers when parsing 304 responses.

@cypress-bot

This comment has been minimized.

Copy link

commented Jun 19, 2019

The code for this is done in cypress-io/cypress#4358, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot

This comment has been minimized.

Copy link

commented Jun 27, 2019

Released in 3.3.2.

@cypress-io cypress-io deleted a comment from cypress-bot bot Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.