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

test,win: enable tcp_connect_error_after_write #4057

Open
wants to merge 1 commit into
base: v1.x
Choose a base branch
from

Conversation

bnoordhuis
Copy link
Member

The test was added in 2012 but never enabled on Windows. Let's see how it fares anno 2023.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if it passes.

@bnoordhuis bnoordhuis force-pushed the connect-after-write-test-windows branch from e1aef45 to bf34061 Compare June 20, 2023 18:08
@bnoordhuis
Copy link
Member Author

For posterity, the write fails with EPIPE instead of EBADF, which I guess is as valid an error as any other?

1: # Output from process `tcp_connect_error_after_write`:
1: # Assertion failed in D:\a\libuv\libuv\test\test-tcp-connect-error-after-write.c on line 68: `UV_EBADF == uv_write(&write_req, (uv_stream_t*) &conn, &buf, 1, write_cb)` (-4083 == -4047)

@vtjnash vtjnash marked this pull request as ready for review December 12, 2023 20:21
@vtjnash vtjnash added the pending-CI Ready to merge, just pending CI label Dec 12, 2023
ASSERT_EQ(r, UV_EBADF);
#endif

r = uv_tcp_connect(&connect_req,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have made a rebase mistake?

2: not ok 314 - tcp_connect_error_after_write
2: # exit code -1073740791
2: # Output from process `tcp_connect_error_after_write`:
2: # Assertion failed in D:\a\libuv\libuv\test\test-tcp-connect-error-after-write.c on line 85: `r == 0` (-4047 == 0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, STATUS_STACK_BUFFER_OVERRUN? I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still happens after a rebase, with changes only to the test file. -4047 is UV_EPIPE and that's a basically impossible return code from uv_run() so I guess that means the buffer overrun status code doesn't come out of completely thin air.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have an ASAN test until it seemed that GithubActions updated something and broke it: #4210

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems that ASAN didn't have anything to report

not ok 314 - tcp_connect_error_after_write
# exit code -1073740791
# Output from process `tcp_connect_error_after_write`:
# Assertion failed in D:\a\libuv\libuv\test\test-tcp-connect-error-after-write.c on line 85: `r == 0` (-4047 == 0)

@bnoordhuis bnoordhuis force-pushed the connect-after-write-test-windows branch from 67514d9 to d905453 Compare January 15, 2024 08:42
The test was added in 2012 but never enabled on Windows. Let's see how
it fares anno 2024.
@vtjnash vtjnash force-pushed the connect-after-write-test-windows branch from d905453 to eeae108 Compare January 21, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-CI Ready to merge, just pending CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants