microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.38k stars 28.61k forks source link

Proxy agent patch prevents extensions from re-using HTTP connections #173861

Closed JadenSimon closed 3 weeks ago

JadenSimon commented 1 year ago

Does this issue occur when all extensions are disabled?: Yes

Steps to Reproduce:

  1. Create a new extension and call https.get or https.request with an agent set to keep connections alive
  2. Change your http.proxySupport setting to override (the default)
  3. Observe that connections are not being reused. This has performance implications as creating sockets is not cheap.

The following code snippet should help demonstrate this problem with notes describing what conditions are needed for connections to persist:

const opt: https.RequestOptions = {
    host: 'example.com',
    port: 443,
    agent: new https.Agent({
        keepAlive: true,
        keepAliveMsecs: 60000,
    }),

    // Explicit headers because something is clobbering the agent's `keepAlive` option
    // Omitting this causes all connections to close regardless of `http.proxySupport`
    headers: { Connection: 'keep-alive' }
}

https.get(opt, r1 => {
    // Without explicit header in `opt` -> `close`
    // With explicit header in `opt` -> `undefined` (implies keep the connection alive)
    console.log('first request connection header ->', r1.headers.connection) 

    // Consume data so we can use the socket again
    r1.on('data', () => {}) 
    r1.on('end', () => {
        https.get(opt, r2 => {
            // `http.proxySupport` set to `override` -> always `false`
            // `http.proxySupport` set to `off` with no header -> `false`
            // `http.proxySupport` set to `off` AND explicit header -> `true`
            console.log('did reuse socket ->', (r2 as any).req.reusedSocket)
        })
    })
}) 

This issue likely originates from here. I'm not entirely sure why an explicit header is needed, but it might be because the proxy patch bypasses logic that would normally set the header. This line appears to be called even when http.proxySupport is set to off.

chrmarti commented 3 weeks ago

@JadenSimon Thanks for the code snippet! We need to pass on the keepAlive flag to the VS Code proxy agent.

Note that triggering the second request in the end listener of the first might miss the existing socket because that hasn't made it to the queue of reusable sockets yet.

Also: VS Code is using a newer Electron version with Node.js 20 and that has keepAlive enabled by default.