Is this still open? Should be reviewed and closed or merged

LGTM

@saghul Any thoughts?

Hi, @oerdnj. Great work and it seems stalled now. Would you mind me picking this up and push forward basing on your work?

@oyyd Sure, I would be delighted if somebody would finish the work into mergeable state. Thanks!

@patrickkh7788 No. I found it was a lot more complicated than imagined :-(

JFTR The MSG_FASTOPEN is not available on macOS (although it defines TCP_FASTOPEN, and the test is broken because of that, so I have to check how to enable TFO on macOS yet.

@oerdnj macOS use connectx with flags CONNECT_DATA_IDEMPOTENT to indicate a socket use TFO.
See http://www.manpagez.com/man/2/connectx/ and
https://github.com/facebook/folly/blob/95ceac6552694ee1dc4f6f289a408aa9eeabc262/folly/detail/SocketFastOpen.cpp#L68

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Any updates on this PR?
This will be very useful for us to add support for TCP Fast Open in Node.js. We are looking for TCP Fast Open support for our node-oracledb driver for Oracle Database.

My recollection (it's been a while) is that this PR is unlikely to get merged anytime soon because:

1. TFO is effectively Linux-only at the moment, and

2. Even on Linux it's not always usable/enabled/etc.

Libraries not supporting a feature because it's not commonly used? Isn't that a circular argument?

I’ve seen the discussion about TFO for njs, so I guess I can rebase this on top of current v1.x and try to get this merged again.

I’ve seen the discussion about TFO for njs, so I guess I can rebase this on top of current v1.x and try to get this merged again.

Great! Thanks @oerdnj . That would be really helpful!

Libraries not supporting a feature because it's not commonly used? Isn't that a circular argument?

No, because that's not the argument. Libuv is a platform abstraction layer. If Linux is the only platform that implements TFO, then it makes no sense to support that in libuv because there's nothing to abstract.

The fact that it doesn't even always work on Linux is just icing on the cake.

I see that Windows support is also available through the ConnectEx API. https://learn.microsoft.com/en-us/windows/win32/api/Mswsock/nc-mswsock-lpfn_connectex
TCP_FASTOPEN can be enabled on the socket.
https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-tcp-socket-options

TIL! This doesn't sound very promising, however:

Note that to make use of fast opens, you should use [ConnectEx](https://learn.microsoft.com/en-us/windows/desktop/api/Mswsock/nc-mswsock-lpfn_connectex) to make the initial connection, and specify data in that function's lpSendBuffer parameter to be transferred during the handshake process. Some of the data in lpSendBuffer will be transferred under the Fast Open protocol.

So seentially we could only support that if you call uv_write before uv_connect actually does the connecting... sounds pretty clunky to use IMHO.

TIL! This doesn't sound very promising, however:

Note that to make use of fast opens, you should use [ConnectEx](https://learn.microsoft.com/en-us/windows/desktop/api/Mswsock/nc-mswsock-lpfn_connectex) to make the initial connection, and specify data in that function's lpSendBuffer parameter to be transferred during the handshake process. Some of the data in lpSendBuffer will be transferred under the Fast Open protocol.

So seentially we could only support that if you call uv_write before uv_connect actually does the connecting... sounds pretty clunky to use IMHO.

There are already places with delayed syscalls, aren’t they? We can delay the connect system call until the first write happens.

There are already places with delayed syscalls, aren’t they? We can delay the connect system call until the first write happens.

That could work!

We can delay the connect system call until the first write happens.

Can you elaborate on how that would work? I can parse that sentence in multiple ways, none which seem feasible or desirable.

I'd assume it means to do something like:

h = uv_tcp_init(...);
uv_tcp_fastopen(h, 1);
uv_connect(...)
uv_write(...)
# connect would happen when uv_write is called, on Windows at least

I'd assume it means to do something like:

h = uv_tcp_init(...);
uv_tcp_fastopen(h, 1);
uv_connect(...)
uv_write(...)
# connect would happen when uv_write is called, on Windows at least

Well, in fact, the PR already does that on Linux. It just have been so long, that I've forgotten what I did here :).

Yeah, that was one of the possible interpretations. :-)

Okay, I see a couple of problems with that:

1. undesirable: also needs to special-case connect + read; more special cases == more ugly

2. undesirable: breaks code that only starts writing from uv_connect_cb; makes uv_tcp_fastopen() not a drop-in replacement

3. undesirable: delaying the connect() call regresses handshake performance

IMO, it'd be best to go in the write-before-connect direction. Clunky but no other undesirable attributes.

Yeah, that was one of the possible interpretations. :-)

Okay, I see a couple of problems with that:

1. undesirable: also needs to special-case connect + read; more special cases == more ugly

The only special case needed would be that you need to start writing (uv_write()) before you start reading (uv_read_start()), otherwise TFO would be ineffective.

Note: uv_try_write() should always fail in not connected yet, so we can process the connect failures.

2. undesirable: breaks code that only starts writing from uv_connect_cb; makes uv_tcp_fastopen() not a drop-in replacement

Why? The uv_connect() would call the callback immediately (without calling system connect()), so in fact, it would be a drop in replacement.

3. undesirable: delaying the connect() call regresses handshake performance

Can you elaborate a bit more? I don't understand what you mean here.

IMO, it'd be best to go in the write-before-connect direction. Clunky but no other undesirable attributes.

Pretty much all downstream code currently is structured like this:

uv_connect(connect_req, handle, [](...) {
  uv_write(...);
  // or:
  uv_read_start(...);
});

That won't work if the connect req doesn't do anything until the first uv_read_start() or uv_write().

Apropos:

undesirable: delaying the connect() call regresses handshake performance

Example: Node's http connection pool opens a bunch of connections to the host but doesn't do anything with them until there's a request ready to be sent.

At the moment uv_tcp_connect() calls connect() straight away. The kernel performs the TCP handshake in the background. The connections are ready by the time they're needed.

With your proposal, the connect() call isn't done until the request is sent. That adds latency.