Skip to content

Fix h2-over-h2 connection proxying

Rodrigo Muino Tomonari requested to merge github/fork/pimterry/h2-kSession-bound-error into main

This fixes #52344 (closed).

Previously, kSession was used on sockets to track the HTTP/2 session happening within that socket, and used on HTTP/2 streams to track the HTTP/2 session containing that stream (i.e. opposite directions in the connection 'stack').

This works fine most of the time, but when a HTTP/2 stream is used as the base for another HTTP/2 connection (using CONNECT proxying over an HTTP/2 stream) these two meanings conflict.

This didn't break until v20.12.0 because in that scenario a JS stream socket is used, and kSession was not passed through that layer until https://github.com/nodejs/node/commit/c50524a9fc19cd2fe6e61bb7f8ad22685024842b#diff-8dd7127678f00bf090a030256bad7ae645834f7100e54154b4fc81c8cbe9c3fc.

This fixes the issue by splitting into kBoundSession (the HTTP/2 session the socket is bound to) and kSession (the HTTP/2 session containing an HTTP/2 stream). I'm fairly sure I've updated all socket-specific use cases correctly to make that change, but it's worth carefully checking the uses of kSession to confirm. Notable parts include:

  • A slight simplification to callTimeout
  • Duplicating the kUpdateTimer call in setStreamTimeout

I think all these changes are exactly equivalent to the previous behaviour, except in the one edge case where something both contains and is contained by an HTTP/2 session.

I'm not sure the setStreamTimeout behaviour is actually correct (I'm not sure I fully understand the wider implications of that logic) but it is equivalent to the previous behaviour anyway so hopefully that's OK for now. Happy to change that to only do one or the other call if only one is actually correct.

Merge request reports