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

[FEATURE] - Ability to setup TCP Socket timeout in workers #1018

Closed
ghost opened this issue Aug 15, 2023 · 11 comments
Closed

[FEATURE] - Ability to setup TCP Socket timeout in workers #1018

ghost opened this issue Aug 15, 2023 · 11 comments
Assignees
Labels
feature request Request for Workers team to add a feature types Related to @cloudflare/workers-types

Comments

@ghost
Copy link

ghost commented Aug 15, 2023

It's not clear what the default TCP socket timeout is nor it's possible to specify one https://github.com/cloudflare/workerd/blob/main/src/cloudflare/internal/sockets.d.ts#L24

Would be awesome to have socketTimeout in the SocketOptions object.

If you point me to how to update documentation for default tcp timeout or point me how to add this I can send a PR

@ghost ghost added the types Related to @cloudflare/workers-types label Aug 15, 2023
@ghost ghost assigned workers-devprod Aug 15, 2023
@kentonv
Copy link
Member

kentonv commented Aug 16, 2023

Hi @dm161,

This is a good idea, but would require a bit more than just a PR in workerd. On Cloudflare, TCP connections go through at outbound proxy which would need to be responsible for setting the timeout. We also need to decide things like what should happen if the connection is answered by another Worker (once Workers gain the ability to accept connections) and thus never actually crosses TCP -- is the timeout ignored or is it emulated? Somewhat related, do we want to express timeouts as literally just a time duration or do we want to provide control over the various low-level TCP keepalive knobs? (I would vote for just an interval, but not sure how complicated it may be to map to the low-level options.)

cc'ing @irvinebroque to think about where this fits on the roadmap.

@ghost
Copy link
Author

ghost commented Aug 16, 2023

thanks for chiming in @kentonv - I would assume there is no timeout as of right now when creating a TCP connection. From the sound of it might require extensive changes to the platform/infra. A solution I was thinking was to wrap the TCP connection into an AbortController in the Cloudflare worker and terminate the connection if a timeout is reached directly from the client code instead.

Slightly hijacking this thread onto an adjacent topic, but still within workerd and network APIs: Where can I open an item for PerformanceResourceTiming support in Workers? Right now fetch is pretty limited if you want to inspect response timings and other breakdowns (DNS time, TTFB).

Thanks again for the great work

@dom96
Copy link
Collaborator

dom96 commented Aug 16, 2023

I'm curious if something as simple as a setTimeout + closure of the Socket when the timeout triggers would work for your use case.

One challenge is that you cannot currently detect when the socket connection is established (as we lack an opened promise in the Socket instance), but maybe for your use case the server always sends some data at the start of the connection anyway, which would let you use that as the point when the connection is established and the timeout can be cancelled. If an opened promise is all that is needed then that might be implementable via just a PR to workerd.

@ghost
Copy link
Author

ghost commented Aug 16, 2023

An opened promise would exactly be the right and optimal compromise for this. Can I help in any way on the codebase? My cpp is rusty but don’t think will be too difficult if it’s just a matter of bubbling it up to the ts code

@jasnell
Copy link
Member

jasnell commented Aug 17, 2023

There are really two timeouts needed, I would think: (a) a time limit for establishing the connection and (b) an idle timeout that closes the connection if it remains idle for too long.

For (a) something as simple as an AbortSignal passed into the connect() call could be sufficient:

// If the connection is not established within 10 seconds, abort the attempt
const s = connect('...', { signal: AbortSignal.timeout(10_000) });

The idle timeout case (b) is more difficult, and as @kentonv suggests requires deeper work in both workerd and the internal infrastructure.

@ghost
Copy link
Author

ghost commented Aug 17, 2023

agreed @jasnell - keep similar API as fetch so we can propagate arbitrary cancellation signals. A bit like how you propagate wth Go context.Context

@dom96
Copy link
Collaborator

dom96 commented Aug 17, 2023

True, we can match fetch here. Though there are other use cases out there for the opened promise than just timeouts (and the websocket API as it stands right now supports it too) so it may be worth implementing in any case.

@dm161 if you want to give writing a PR a go: following how the closed promise is implemented (in sockets.c++) is a good route to take. You likely want to resolve the new opened promise you introduce inside the handleProxyStatus method.

The idle timeout case (b) is more difficult, and as @kentonv suggests requires deeper work in both workerd and the internal infrastructure.

Why is that? Similarly to (a) wouldn't a naive approach of closing the socket when no data is received work well for most use cases?

@jasnell
Copy link
Member

jasnell commented Aug 17, 2023

Why is that? Similarly to (a) wouldn't a naive approach of closing the socket when no data is received work well for most use cases?

I suppose it could. Every bit of data transmitted or received could reset a timer.

@ghost
Copy link
Author

ghost commented Aug 19, 2023

Fixed on the client side for now using setTimeout and Promise.race as follows

router.get('/tcp', async (request, env) => {
  const { searchParams } = new URL(request.url)
  let host = searchParams.get('host')
  if (host === null) {
    return;
  }
  let port = searchParams.get('port')
  if (port === null) {
    return;
  }
  let start = Date.now();
  
  const bufferSize = 1024*1024;
  const tcpTimeout = 5000;

  async function socketConnect(bufferSize) {
    const socket = connect({ hostname: host, port: port });
    const writer = socket.writable.getWriter();
    const encoder = new TextEncoder();
    const encoded = encoder.encode(`GET / HTTP/1.1\r\nHost: ${host}\r\nConnection: close\r\n\r\n`);
    await writer.write(encoded);
    await writer.close();
    const reader = socket.readable.getReader({ mode: 'byob' });
    const data = await reader.readAtLeast(bufferSize, new Uint8Array(new ArrayBuffer(bufferSize), 0, bufferSize));
    const dec = new TextDecoder("utf-8");
    const output = dec.decode(data.value);
    await socket.close();

    return json({ latency: Date.now() - start, timeout: false, data: output });
  }

  async function socketTimeout(tcpTimeout) {
    await wait(tcpTimeout);
    return json({ latency: Date.now() - start, timeout: true });
  }

  return await Promise.race([socketTimeout(tcpTimeout), socketConnect(bufferSize)]);
});

async function wait(ms) {
  return new Promise(function (resolve, reject) {
    setTimeout(resolve, ms)
  })
}

Still having opened Promise would be useful i will dig more in the code base if it's easy to add. Thanks all

@ghost ghost changed the title Feature - Ability to setup TCP Socket timeout in workers [FEATURE] - Ability to setup TCP Socket timeout in workers Aug 19, 2023
@lyc8503
Copy link

lyc8503 commented Oct 28, 2023

An opened promise would exactly be the right and optimal compromise for this. Can I help in any way on the codebase? My cpp is rusty but don’t think will be too difficult if it’s just a matter of bubbling it up to the ts code

One more vote for an opened promise, which could helps developers get more control of the timing and connection status.

@jasnell jasnell added the feature request Request for Workers team to add a feature label Oct 28, 2023
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Oct 28, 2023
@dom96
Copy link
Collaborator

dom96 commented Jan 24, 2024

opened is now implemented

@dom96 dom96 closed this as completed Jan 24, 2024
@github-project-automation github-project-automation bot moved this from Untriaged to Done in workers-sdk Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature types Related to @cloudflare/workers-types
Projects
None yet
Development

No branches or pull requests

5 participants