Half close port forwarding connections to fix hangs

[cmbrose]

Fixes: #6842

See the comment here for a full explanation of the bug: #6842 (comment)

The core change here is to close the write side of the net.Conn to the client in order to signal that client that the response is complete.

The base net.Conn however doesn't expose a function to close only the writer, so everything is changed to use net.TCPListener which does. The bulk of the change is to support that a little more cleanly.

[cmbrose]

👋 @josebalius I know this isn't your space anymore, but you're probably the expert on this code still 😅 If you still have context, would appreciate confirmation on my understanding of how the flow works - thanks!

[josebalius]

Makes sense, although not sure why we need a half closer, can't we close both the read and write?

[mislav]

While you're working on this @cmbrose, note that port forwarding tests have been disabled in CI due to being flakey

cli/pkg/liveshare/port_forwarder_test.go

Line 37 in 71ec2c4

t.Skip("fails intermittently in CI: https://github.com/cli/cli/issues/5338")

and that some other ports-related tests might be flakey as well #6858 (comment)

[cmbrose]

not sure why we need a half closer, can't we close both the read and write?

@josebalius we need to keep the read side open so that the local side can still write a reply after the codespace finishes the response. That flow would look like this:

1. local app sends a request to forwarded port
2. gh receives the request on the TCP connection io.Reader and forwards to the ssh channel io.Writer
3. ssh channel sends the request to the codespace, which responds and closes its own TCP write connection
4. ssh channel in gh receives the codespace response in its io.Reader and forwards the reply to the TCP connection
5. ssh channel's reader reads EOF and exits the io.Copy call
6. TCP connection closes its io.Writer signaling to the local app that the response is complete
7. local app sees the response is complete and sends one final message back to the codespace through the TCP connection's still-open reader which forwards to the ssh channel's still-open writer which forwards to the codespace's still-open reader
8. no reply can come from the codespace, so the local app closes its own writer
9. TCP connection reader reads EOF and exits the io.Copy call
10. ssh channel closes its io.Writer which terminates both the TCP connection and ssh channel

This PR specifically adds the logic in step 6. Without that new logic, step 8 can hang forever as the app may think the codespace might have data to send still.

The need to only half close the TCP connection is to support the scenario in step 7. If we fully closed the connection in step 6 then the local app couldn't send additional data to the codespace after getting the response.

[cmbrose]

port forwarding tests have been disabled in CI due to being flakey

Thanks for the heads up! Ran locally and looks good

[aditya30394]

Looks good to me

[trent-j]

LGTM

[thomas-sickert]

Just had one question about reusing a method, other than that, functionality LGTM 👍