Fixes cypress crash (ECONNRESET) on socket hang up during attempted socket upgrade

[wijwoj]

Never done an actual pull request for an open source project, so let me know what is required to get this merged in.

This fixes the problems I was having described in here: #556

[CLAassistant]

All committers have signed the CLA.

[wijwoj]

Also I realise something should probably done with the error, so let me know what you think is an appropriate course of action.

[brian-mann]

Hello!

Thanks for this PR. There are a couple server unit tests failing (likely due to stubbing out the object).

The bigger thing we'll need is a test demonstrating what exactly this is fixing. Modifying any behavior in the proxy is a really big deal and can have reverberating effects throughout the whole system.

Can you explain in human terms what you're doing manually on your end to reproduce this / ensure it is fixed? What is the desired outcome? From there we can probably come up with a way to write a test for it.

[brian-mann]

@wijwoj nevermind - I see that in your associated issue it is well described. Let me take a look at that and see if we can come up with a test ensuring that it demonstrates the crash.

Duplicate

[wijwoj]

Thanks for the response @brian-mann . I don't fully understand why (one) of my test sites triggers this error so consistently. But if you can generate/fake any kind of upstream error whilst in the socket upgrade the cypress process dies, therefore I think it makes sense to try and defend against it. I did have a look at your unit tests but I'm guessing you don't run them on windows as I was getting a pile of fails locally.

I would love to get this merged in as putting a forked version in our repo was a bugger due to the size and number of files.

I did also wonder if the error should be bubbled up so the user could choose what to do with it.

Happy to help in any way to move this forward.

[brian-mann]

Correct, the unit tests are not run in Windows. We only run the complete and full e2e tests. This was due to us writing all of our tests in a way that doesn't work with Windows and we never updated them all.

I have to do some research on the best approach for recovering. It's possible we just want to transparent pass the errors through and let the browser (user agent) decide how best to proceed.

There is another issue open detailing this as well. The proxy itself adds its own error handling mechanisms in several places and this actually causes problems because the browser thinks there wasn't an error. Chrome is naturally built based on the error code to know when / how to retry - and that should likely be our default behavior.

[wijwoj]

So this was where I was looking on how to deal with these errors http-party/node-http-proxy#264, which strangely brought up an issue with cypress that I had looked at a couple of days ago #957. Was that the other issues you mentioned, or was it another one? I'm going to have a crack at creating a test for my case.

I'm ashamed to admit I've barely touched linux for a couple of years. Can I ask what was your standard build/test environment?

[wijwoj]

@brian-mann Is that last failure caused by my commit? Seems fairly unrelated?

[nateabele]

It'd be great to get this merged. We're currently patching this change into Cypress manually in CI to get our test suite to run without crashing.

[jennifer-shehane]

Hey, sorry, yes, we are very busy at the moment and hoping to get in a release as soon as possible! A lot of moving parts since it's a desktop application deployment.

[nateabele]

@jennifer-shehane Thanks!

[wijwoj]

Added a very simple unit test and had a look at what other were doing with these sort of errors. Let me know if you would like any other changes.

[wijwoj]

So I tried to do something with the error eg res.status(500).end() or even passing back the error, but due to issues like http-party/node-http-proxy#968 it doesn't work without a hack, so I think we should just end the response as I had it originally

[brian-mann]

I spent about 9 hours O_O on this today. I believe we're now correctly handling websocket upgrade failures, providing some custom headers (to make this clear) and sending back 502 status code since it's not possible to transparently handle ECONNREFUSED errors exactly the same when you have a valid proxy in the middle.

I wrote some e2e + integration tests for this too... if all tests pass we'll merge this in. @mxstbr this will fix your issue in spectrum.chat.

Instead of Cypress crashing it'll gracefully handle the error, close the underlying socket connection and happily move on.

[matt-psaltis]

Just wanted to send my thanks to everyone looking into this issue and spending time to resolve it - our team appreciates it.

[wijwoj]

Crikey. What a slog. Thanks @brian-mann. Really appreciate the efforts and your team's responsiveness to the community. Makes a real difference to know that there are people committed to getting stuff done here. Thanks again.

[nateabele]

Just wanted to send my thanks to everyone looking into this issue and spending time to resolve it - our team appreciates it.

Seconded.