Skip to content

core-net: client: don't fan out parallel connects on foreign event loops#3621

Closed
saghul wants to merge 1 commit into
warmcat:mainfrom
saghul:fix-win32-connect-poller-foreign-loop
Closed

core-net: client: don't fan out parallel connects on foreign event loops#3621
saghul wants to merge 1 commit into
warmcat:mainfrom
saghul:fix-win32-connect-poller-foreign-loop

Conversation

@saghul

@saghul saghul commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

The win32 async connect-completion poller (lws_client_win32_conn_async_check,
scheduled unconditionally on Windows in lws_client_connect_3_connect) re-enters
connect_3 with no specific fd. When it fires while the primary connect is still
in progress and DNS returned more than one address, the "else" branch falls
through and opens an additional parallel connect socket on the same wsi
(is_parallel = lws_socket_is_valid(wsi->desc.sockfd)).

Parallel connect sockets are only serviceable on the built-in "poll" core loop,
which polls every fd in pt->fds itself. A foreign event loop (event_lib_custom:
libuv/libev/libevent/glib/sd) tracks one fd per wsi via its io/sock_accept ops
and cannot poll the extra parallel sockets; adopting one corrupts the wsi's
fds-table bookkeeping. Observed on a libuv-based custom event lib on Windows:
__remove_wsi_socket_from_fds() trips

assert(m == LWS_NO_FDS_POS || (m >= 0 && (unsigned int)m < pt->fds_count))

(pollfd.c) because position_in_fds_table is left out of range; in NDEBUG builds
the same out-of-range index is used silently (OOB on pt->fds[]), and the
detached primary TLS socket stalls until SECS_WAITING_FOR_SSL ("Timed out
waiting SSL").

The happy-eyeballs timer is already gated on the "poll" loop where it is
scheduled; the win32 connect-check is scheduled regardless of the loop, so gate
the parallel fan-out itself: only fall through to spawn a parallel socket when
the context's event loop is the built-in "poll" core. Foreign loops fall back to
sequential connect, driven by their own POLLOUT, exactly as before happy
eyeballs.

The win32 async connect-completion poller (lws_client_win32_conn_async_check,
scheduled unconditionally on Windows in lws_client_connect_3_connect) re-enters
connect_3 with no specific fd. When it fires while the primary connect is still
in progress and DNS returned more than one address, the "else" branch falls
through and opens an additional *parallel* connect socket on the same wsi
(is_parallel = lws_socket_is_valid(wsi->desc.sockfd)).

Parallel connect sockets are only serviceable on the built-in "poll" core loop,
which polls every fd in pt->fds itself. A foreign event loop (event_lib_custom:
libuv/libev/libevent/glib/sd) tracks one fd per wsi via its io/sock_accept ops
and cannot poll the extra parallel sockets; adopting one corrupts the wsi's
fds-table bookkeeping. Observed on a libuv-based custom event lib on Windows:
__remove_wsi_socket_from_fds() trips

  assert(m == LWS_NO_FDS_POS || (m >= 0 && (unsigned int)m < pt->fds_count))

(pollfd.c) because position_in_fds_table is left out of range; in NDEBUG builds
the same out-of-range index is used silently (OOB on pt->fds[]), and the
detached primary TLS socket stalls until SECS_WAITING_FOR_SSL ("Timed out
waiting SSL").

The happy-eyeballs timer is already gated on the "poll" loop where it is
scheduled; the win32 connect-check is scheduled regardless of the loop, so gate
the parallel fan-out itself: only fall through to spawn a parallel socket when
the context's event loop is the built-in "poll" core. Foreign loops fall back to
sequential connect, driven by their own POLLOUT, exactly as before happy
eyeballs.
@sonarqubecloud

Copy link
Copy Markdown

@lws-team

Copy link
Copy Markdown
Member

Ugh.... I didn't notice that we were in the position the HE races only supported poll()... I'm working on extending HE client ops to work on all the event loops the same.

@saghul

saghul commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

We seem to be :-) I worked around it in txiki.js, but having the support extended and working would be great!

Would you like me to close this?

@lws-team

Copy link
Copy Markdown
Member

No it's fine to leave it open thanks.

@saghul saghul closed this Jun 30, 2026
@saghul

saghul commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

I see you fixed it in main, I'll test it shortly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants