🐛 Bug Report — Runtime APIs connect() and write to an closed port doesn't throw an error

[lyc8503]

I am trying to initiate a TCP connection to some hostname:port to test if that port is open.

const socket = connect({ hostname: '1.1.1.1', port: 1234 })
const writer = socket.writable.getWriter()
await writer.write(new TextEncoder().encode("PING\n"))
await socket.close()

In the code above, I open a tcp connection to a closed port 1234 on 1.1.1.1, writes something to that and await for the result. However it doesn't throw any errors.

So I assume that when calling writer.write it doesn't actually send the data, instead it resolved the promise almost immediately.

Now my question is how to determine whether the TCP handshake is done and data is actually sent?

[dom96]

Errors should show up in the closed promise. So try await socket.closed after closing it.

[lyc8503]

I tried to append await socket.closed at the end of my snippet, and also added a timer.

const startTime = Date.now()

const socket = connect({ hostname: '1.1.1.1', port: 1234 })
const writer = socket.writable.getWriter()
await writer.write(new TextEncoder().encode("PING\n"))
await socket.close()
await socket.closed

console.log(Date.now() - startTime)

It doesn't throw an error and prints 0 to the console.

I don't think it can finish TCP handshake in less than 1ms, so I guess it haven't completed the handshake when it resolves the promise.

[lyc8503]

Also, when I try to run telnet 1.1.1.1 1234 from a random linux machine, I got:

$ telnet 1.1.1.1 1234    
Trying 1.1.1.1...
telnet: Unable to connect to remote host: Connection timed out

so I expect connect (or at least write) to throw a ConnectionRefused / ConnectionTimeout / ClosedStream error when I call await connect/write

[dom96]

There was some recent fixes that may help with this, if you can grab workerd HEAD and run your script under that I'd be curious if you can still reproduce this. Be sure to do await writer.close() as well.

[lyc8503]

Actually I haven't setup workerd locally and I am now just using Cloudflare Workers. I may try running workerd by myself.

and btw I am wondering when the fix will be implemented in Cloudflare? Is there a schedule for that?

[dom96]

Depends what you mean by "in Cloudflare", it should already be implemented in Cloudflare Workers that are running on our edge. For wrangler dev I believe we need to wait for a new workerd to be released.

[lyc8503]

I just tried again on Cloudflare Workers, but the issue seems to remain the same.

import { connect } from "cloudflare:sockets";

export default {
  async fetch(request, env, ctx) {

    const startTime = Date.now()

    const socket = connect({ hostname: '1.1.1.1', port: 1234 })
    const writer = socket.writable.getWriter()
    await writer.write(new TextEncoder().encode("PING\n"))
    await socket.close()
    await socket.closed

    return new Response(JSON.stringify(Date.now() - startTime))
  },
};

The code above returns 0 instead of throwing anything.

To verify, you can visit https://worker-noisy-snow-c851.uptimeflare.workers.dev/
The exact source code for this worker is the code above, character for character. I think you can easily reproduce it in your Cloudflare account by creating a worker and copy-paste the code.

So it seems that at least the issue is not fixed now at Cloudflare Workers?

[jasnell]

FWIW, Having the await socket.closed after await socket.close() will do nothing really since those are essentially the same promise. Once socket.close() resolved, socket.closed should also be resolved.

@dom96 ... I haven't looked at it to say for sure but my initial guess is that the error reporting that the connection couldn't be established is not making it's way into the WritableStream. When that fails it should trigger an abort(err) on the WritableStream that causes any pending writes to abort.

That said, it could be that we're flushing the write (and resolving that promise) before the connection error is returned back up to the Socket, in which case aborting the WritableStream would be silent in this case and we'd need to make sure that error propagates to the socket.close() promise. (that said, should calling socket.close() on a Socket that failed to connect be an error or should it just be a non-op?)

[dom96]

Ahh, yeah, my bad.

@lyc8503 as James mentioned, calling await socket.close() will resolve the closed promise of the socket, so your latest code just ends up ignoring the error. I just tried the following:

import { connect } from "cloudflare:sockets";

export default {
  async fetch(request, env, ctx) {

    const startTime = Date.now()

    const socket = connect({ hostname: '1.1.1.1', port: 1234 })
    const writer = socket.writable.getWriter()
    await writer.write(new TextEncoder().encode("PING\n"))
    await socket.closed

    return new Response(JSON.stringify(Date.now() - startTime))
  },
};

And it results in an error as expected. Is this what you were after? If so we can close this issue.

[dom96]

that said, should calling socket.close() on a Socket that failed to connect be an error or should it just be a non-op?

As for this, let's discuss in the spec repo. I'll make an issue there.

[kentonv]

I'd strongly argue that close() should not complete successfully unless all previous writes actually successfully went out to the socket.

[lyc8503]

Ahh, yeah, my bad.

@lyc8503 as James mentioned, calling await socket.close() will resolve the closed promise of the socket, so your latest code just ends up ignoring the error. I just tried the following:

import { connect } from "cloudflare:sockets";

export default {
  async fetch(request, env, ctx) {

    const startTime = Date.now()

    const socket = connect({ hostname: '1.1.1.1', port: 1234 })
    const writer = socket.writable.getWriter()
    await writer.write(new TextEncoder().encode("PING\n"))
    await socket.closed

    return new Response(JSON.stringify(Date.now() - startTime))
  },
};

And it results in an error as expected. Is this what you were after? If so we can close this issue.

Thanks! This solves the problem for me, so I am closing this issue.

BTW its not until now that I've realized actually it's documented here, I was just using it as a "common" socket API but there was something different. 😥

[dom96]

I'd strongly argue that close() should not complete successfully unless all previous writes actually successfully went out to the socket.

Agreed, will get that fixed.

[dom96]

#1369