[PATCH v2 0/1] MR10044: Draft: server: Clear connection failure events in IOCTL_AFD_WINE_CONNECT.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53574 See details in the bug report. [Testbot run with this patch](https://testbot.winehq.org/JobDetails.pl?Key=161764) -- v2: server: Clear connection failure events in IOCTL_AFD_WINE_CONNECT. https://gitlab.winehq.org/wine/wine/-/merge_requests/10044
From: Bernhard Übelacker <bernhardu@mailbox.org> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53574 --- dlls/ws2_32/tests/sock.c | 33 +++++++++++++++++++++++++++++++++ server/sock.c | 4 ++++ 2 files changed, 37 insertions(+) diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 8d589c63ea3..f71b6d04dea 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -6875,6 +6875,39 @@ static void test_connect_events(struct event_test_ctx *ctx) closesocket(server); closesocket(listener); + + /* Test events getting cleared on second connect after connection got refused. + * w10pro64 sometimes takes over 2 seconds for an error to be reported, + * so make the test interactive-only. */ + //if (winetest_interactive) //disabled to run with gitlab, until leaving draft state + { + struct sockaddr_in invalid_addr; + memset( &invalid_addr, 0, sizeof(invalid_addr) ); + invalid_addr.sin_family = AF_INET; + invalid_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + invalid_addr.sin_port = htons(255); + + client = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ok(client != -1, "failed to create socket, error %u\n", WSAGetLastError()); + + select_events(ctx, client, FD_ACCEPT | FD_CLOSE | FD_CONNECT | FD_OOB | FD_READ | FD_WRITE); + check_events(ctx, 0, 0, 0); + check_events(ctx, 0, 0, 0); + + /* attempt to connect to closed port */ + ret = connect(client, (struct sockaddr*)&invalid_addr, sizeof(invalid_addr)); + ok(ret == SOCKET_ERROR, "expected SOCKET_ERROR, got %d\n", ret); + check_events(ctx, MAKELONG(FD_CONNECT, WSAECONNREFUSED), 0, 4000); + check_events(ctx, 0, 0, 0); + + /* try again to connect, should behave the same */ + ret = connect(client, (struct sockaddr*)&invalid_addr, sizeof(invalid_addr)); + ok(ret == SOCKET_ERROR, "expected SOCKET_ERROR, got %d\n", ret); + check_events(ctx, MAKELONG(FD_CONNECT, WSAECONNREFUSED), 0, 4000); + check_events(ctx, 0, 0, 0); + + closesocket(client); + } } /* perform a blocking recv() even on a nonblocking socket */ diff --git a/server/sock.c b/server/sock.c index 7785d3c7706..38ab92d3447 100644 --- a/server/sock.c +++ b/server/sock.c @@ -2733,6 +2733,10 @@ static void sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) return; case SOCK_UNCONNECTED: + sock->pending_events &= ~AFD_POLL_CONNECT_ERR; + sock->reported_events &= ~AFD_POLL_CONNECT_ERR; + break; + case SOCK_CONNECTIONLESS: break; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10044
v2: [Testbot run with this patch](https://testbot.winehq.org/JobDetails.pl?Key=161893) - Use port 255. - Call check_events twice. - Clear the error only with SOCK_UNCONNECTED, remove the `if (sock->type == WS_SOCK_STREAM)`. - Clear only the AFD_POLL_CONNECT_ERR in `reported_events`. - Clear the AFD_POLL_CONNECT_ERR also in `pending_events`. - Run this test only with winetest_interactive. - Change patch title to better reflect what it is about. TODO: - Add test: "I.e. if you fail connection, don't check events, and then successfully connect, I would expect that the connection failure is not reported. Ideally this too should be tested." - Add test: "I'm also curious if trying to connect with an invalid address will report this behaviour. If not, we want to clear reported_events under that check." -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10044#note_129551
On Thu Feb 12 21:25:46 2026 +0000, Elizabeth Figura wrote:
Because Windows sometimes takes a long time to fail connection (and because we want our test suite to complete relatively quickly), we hide similar tests behind winetest_interactive. I think that should be done here too. We also usually use port 255, which is defined as invalid. I think we should call check_events() twice, as is generally done elsewhere. I'm not expecting any surprises, but it's good to be sure. We should probably also be clearing pending_events here, as we do almost everywhere else. I.e. if you fail connection, don't check events, and then successfully connect, I would expect that the connection failure is *not* reported. Ideally this too should be tested. Putting the assignment inside of a block containing both SOCK_UNCONNECTED and SOCK_CONNECTIONLESS but then checking for SOCK_STREAM anyway is redundant. Easier is just to put it under SOCK_UNCONNECTED only. I'm also curious if trying to connect with an invalid address will report this behaviour. If not, we want to clear reported_events under that check. There's a part of me that dislikes setting to 0 and not simply clearing AFD_POLL_CONNECT_ERR, but I guess they're equivalent in this case. The latter might still be more declarative, or intuitively correct. The title is confusing; I see why you say "reuse" but that's not the first thing I think of (in particular I think of either TF_REUSE, or the couple of cases where we have to recreate the underlying Unix socket.) I would instead say "Clear events in IOCTL_AFD_WINE_CONNECT" or (assuming the above change) "Clear connection failure events in IOCTL_AFD_WINE_CONNECT". In the future, when making bug-fix changes like this, it is helpful to separate the tests into a first patch, so it is abundantly clear what the commit is fixing. Hello Elizabeth, thanks for your great feedback. I pushed v2 that addresses a few of the raised issues. But I have trouble adding the additional tests. What would an invalid address look like?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10044#note_129554
Hello Elizabeth, thanks for your great feedback. I pushed v2 that addresses a few of the raised issues. But I have trouble adding the additional tests. What would an invalid address look like?
Cases where sockaddr_to_unix returns zero. It's probably enough to test just one such case, e.g. if the address length is too small, or if the family is invalid. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10044#note_129842
participants (3)
-
Bernhard Übelacker -
Bernhard Übelacker (＠bernhardu)
-
Elizabeth Figura (＠zfigura)