fix(ext/net): return an error from startTls and serveHttp if the original connection is captured elsewhere

[magurotuna]

Fixes #9360
Fixes #13345
Fixes #13926
Fixes #16241

Overview

This PR removes the calls to expect() on std::rc::Rc, which caused Deno to panic under certain situations. We now return an error if Rc is referenced by other variables.

The point is, our assumption - we have exclusive access to the underlying connection when upgrading it to upper-layer connection - was actually wrong.

Detail

const hostname = "localhost";
const port = 25432;

// Start listening on server
const tcpListener = Deno.listen({ hostname, port });

// Establish TCP connection on both server and client
const [serverConn, clientConn] = await Promise.all([
  tcpListener.accept(),
  Deno.connect({ hostname, port }),
]);

const buf = new Uint8Array(128);
// [client] Try to read data from the server (without awaiting)
const readPromise = clientConn.read(buf);
// [client] Upgrade to TLS connection
await Deno.startTls(clientConn, { hostname });

Running the above code we can see Deno always panics. This is because clientConn is captured by ongoing promise (readPromise) although Deno.startTls assumes that it has exclusive access to the connection.

One solution would be we ensure that all of the other promises capturing the connection have been completed before calling startTls. This PR basically follows this approach - instead of panicking in such situations, it returns a bad resource error to indicate that you need to check other promises. I'm not sure if the error should be classified as a bad resource though.

This issue is beyond startTls. Other builtin functions such as serveHttp are also suffering from the issue for the same mechanism.

Looking at this issue a bit further, the root cause is that the operation of upgrading a TCP connection to TLS requires a TCP one to be consumed i.e. after upgrading it, the original TCP connection is deleted from the resource table and no longer usable. The requirement for consumption of the original connection is the reason why exclusive access is needed. In fact, this behavior has caused trouble and there's a issue about it:

• Cannot use the original Deno.Conn after Deno.serveHttp #11744

To address this root cause, I think we could clone the original connection (for example using std::net::TcpStream::try_clone) instead of consuming it, which allows us to retain the original connection in the resource table and to add an upgraded connection as a new resource. In this way, we don't have to consume the original connection and thus the upgrade process won't need exclusive access to the original one.

The problem with this approach is tokio's TcpStream doesn't have try_clone method. Maybe we could implement the clone logic using a raw file descriptor together with syscall (like dup) but it might be platform-specific and need further investigation to see if it works as we expect. So I didn't go with this approach in this PR.

[CLAassistant]

All committers have signed the CLA.

[imcotton]

Thanks for the investigation, in case of issue 11744 mentioned on above, I was hoping that at least the readable and writable from Conn works since they're representing modern interface of web streams, thus currently I'm stuck with deprecated std@0.117.0/http/server_legacy as workaround, but also worried its removal in upcoming Deno v2.

[bartlomieju]

LGTM, good fix!

[scarletquasar]

Is there any problem occurring on windows?

[magurotuna]

We ignore this test particular test case on windows because it's about unix socket which Deno supports only on unix.

[magurotuna]

Thanks for proofreading! @bartlomieju