fix(websocket): handle errors in handleUpgrade #823
Conversation
| function isSocketLike(obj: any): obj is Socket { | ||
| return obj && typeof obj.write === 'function' && !('writeHead' in obj); | ||
| } |
There was a problem hiding this comment.
We can't just check for instanceof Socket since per the documentation, the user may be able to provide a different socket implementation:
This event is guaranteed to be passed an instance of the <net.Socket> class, a subclass of <stream.Duplex>, unless the user specifies a socket type other than <net.Socket>.
| @@ -97,15 +97,14 @@ describe('E2E WebSocket proxy', () => { | |||
|
|
|||
| describe('with router and pathRewrite', () => { | |||
| beforeEach(() => { | |||
| // override | |||
| proxyServer = createApp( | |||
| const proxyMiddleware = createProxyMiddleware({ | |||
There was a problem hiding this comment.
This was a bug with the existing tests: because it didn't reassign proxyMiddleware, the proxyMiddleware.upgrade access a few lines after this was reading from the wrong object.
|
@chimurai 👋 can I do anything to help this along? |
|
Sorry for the late reply. I'll need some time to review 🙏 |
|
@chimurai thanks! |
|
@chimurai is there any chance you'd be able to review soon? If so, I'll resolve the merge conflicts with master. |
Description
When there is an error from user-provided code (
pathFilter,rewritePath, orrouter), that error is now handled and exposed to the user viaproxy.emit('error', ...).error-response-pluginwas also updated to handle the fact that it might receive a socket. If it does, it will callsocket.destroy().Motivation and Context
See #822 for a description of the issue being fixed. This PR closes #822.
How has this been tested?
I added a new test case. It fails on master, and works correctly with my new changes.
Types of changes
I'm split on whether or not this should be considered a breaking change. Folks could have written code assuming that the error handler would always get a
Responseas the third argument, whereas now it would get aSocket. That said, the TypeScript types fromhttp-proxydo reflect that that could be aSocket, so it shouldn't come as too much of a surprise.Checklist: