Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

πŸ› Bug Report β€” Closing TCP socket that didn't finish connection takes ~20 seconds, during which it still counts towards max outbound connection limit #2060

Open
mornveig opened this issue Apr 26, 2024 · 4 comments
Labels
api feature request Request for Workers team to add a feature socket workers-runtime

Comments

@mornveig
Copy link

Hi team,

Closing TCP socket that didn't finish connection (for example host is unresponsive or port is closed) takes about ~21 seconds both locally and when deployed as cloudflare worker. During that time TCP socket does count towards outbound connection limit, and there seems to be no way currently to forcibly close the socket.

The following code could be used to check the time it takes to close the socket:

import { connect } from 'cloudflare:sockets';

export default {
	async fetch(request, env, ctx) {
		var start_time = Date.now();
		async function socket_connect_close() {
			var socket = connect({ hostname: "8.8.8.8", port: 9999 });
			await socket.close();
			return Date.now() - start_time;
		}
		return new Response(JSON.stringify({'close_time': await socket_connect_close()}), {headers: {"Content-Type": "application/json"}});
	},
};

If connection limit is reached, properly closing unresponsive connection delays all new connections by 21 seconds which sometimes is undesirable.
It'd be great if there was a way to control connect/close timeout for TCP sockets.

@abn5x
Copy link

abn5x commented Jan 8, 2025

Same issue here...

@kentonv
Copy link
Member

kentonv commented Jan 13, 2025

close() is intended to perform a clean shutdown including completing all previous writes, so this is working as intended. Instead, abort() should be used to destroy the socket promptly.

It looks like Socket is missing an abort() method to force dirty shutdown. However, WritableStream has one. Does socket.writable.abort() do what you need?

Either way I think we should probably add abort() to the Socket itself.

@jasnell
Copy link
Member

jasnell commented Jan 13, 2025

@dom96 ... we should look at the Socket API to figure out an abrupt abort. I remember this being discussed before at some point but it wasn't clear if it would be needed so it was punted on. I just don't recall who that conversation was with ;-) ... Fundamentally I think the behavior of socket.abort(err) should be to simply defer to socket.writable.abort(err); socket.readable.cancel(err);, the combination of which should abruptly terminate the socket

@jasnell jasnell added feature request Request for Workers team to add a feature api socket labels Jan 13, 2025
@dom96
Copy link
Collaborator

dom96 commented Jan 14, 2025

Sounds good. We'll have to add this to our sockets spec as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api feature request Request for Workers team to add a feature socket workers-runtime
Projects
None yet
Development

No branches or pull requests

6 participants