Skip to content

vsock: add DialTimeout#40

Open
BetaXOi wants to merge 1 commit intomdlayher:mainfrom
BetaXOi:timeout
Open

vsock: add DialTimeout#40
BetaXOi wants to merge 1 commit intomdlayher:mainfrom
BetaXOi:timeout

Conversation

@BetaXOi
Copy link

@BetaXOi BetaXOi commented May 8, 2020

We need to use Dial with timeout feature in Kata, see kata-containers/runtime#1917 (comment)

Signed-off-by: Ning Bo ning.bo9@zte.com.cn

Copy link
Owner

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

Thanks for this! The Go stable CI failure appears to be related to an outdated dependency so I've pushed a new commit. Could you please rebase on master?

In addition, if possible, I would love to see a test with a very short dial timeout to verify that the applied timeout parameter does take effect.

@BetaXOi
Copy link
Author

BetaXOi commented May 8, 2020

OK, i will try to add a new test case for timeout

Signed-off-by: Ning Bo <ning.bo9@zte.com.cn>
@BetaXOi
Copy link
Author

BetaXOi commented May 11, 2020

I have rebased and add a test for timeout.
But the test case has some problems:

  1. The effective minimum timeout is DIV_ROUND_UP (tv.tv_usec, (1000000 / HZ)) see https://github.com/torvalds/linux/blob/master/net/vmw_vsock/af_vsock.c#L1571
  2. Due to the problem above, test cases sometimes will fail

[root@localhost vsock]# for i in seq 10; do go test -v -race -run TestIntegrationConnTimeout; done
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.01s)
PASS
ok github.com/mdlayher/vsock 0.052s
=== RUN TestIntegrationConnTimeout
TestIntegrationConnTimeout: integration_linux_test.go:235: connect with a very short dial timeout expect ETIMEDOUT, but got
--- FAIL: TestIntegrationConnTimeout (0.00s)
FAIL
exit status 1
FAIL github.com/mdlayher/vsock 0.032s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.049s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.042s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.048s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.046s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.046s
=== RUN TestIntegrationConnTimeout
TestIntegrationConnTimeout: integration_linux_test.go:235: connect with a very short dial timeout expect ETIMEDOUT, but got
--- FAIL: TestIntegrationConnTimeout (0.00s)
FAIL
exit status 1
FAIL github.com/mdlayher/vsock 0.033s
=== RUN TestIntegrationConnTimeout
TestIntegrationConnTimeout: integration_linux_test.go:235: connect with a very short dial timeout expect ETIMEDOUT, but got
--- FAIL: TestIntegrationConnTimeout (0.00s)
FAIL
exit status 1
FAIL github.com/mdlayher/vsock 0.031s
=== RUN TestIntegrationConnTimeout
--- PASS: TestIntegrationConnTimeout (0.00s)
PASS
ok github.com/mdlayher/vsock 0.050s
[root@localhost vsock]#

@mdlayher
Copy link
Owner

Thank you for the PR and I apologize for the delay: I am mostly on hiatus from open source software development at the moment. However I intend to address this and the other outstanding PRs as part of #45 and will follow up once I'm able to do so. Thanks again.

@mdlayher
Copy link
Owner

mdlayher commented Feb 4, 2022

Hi there, I apologize for the delay. With the advent of v1.0.0, perhaps it might make sense to add timeout as a parameter to the vsock.Config struct. I made the decision to share the config fields between Dial and Listen so I'd want to see if a timeout might make sense on that side too.

I'll do some investigation into what socket options are available too.

@ajsinclair
Copy link

I wonder if DialContext would be more flexible. It would also match the net and gRPC packages:

@sheepa
Copy link

sheepa commented Sep 18, 2025

How would this be done in the current version? Is there some other way to do this?

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.

4 participants