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

add connection timeout function #140

Closed
wants to merge 0 commits into from

Conversation

button-chen
Copy link

@button-chen button-chen commented Oct 23, 2024

Hello, the current implementation of Dial does not have a timeout , in some cases we should need to specify a connection timeout. So I added a DialWithContext function.

I tried to write it like this before:

c, err := net.Dial(network, addr)
c.SetReadDeadline(...)   // set
Connect(c, ...)
c.SetReadDeadline(...)   // recover

but this is not guaranteed to be correct, however, because the code in the Connect function: go readLoop(c, reader) -->

f, err := reader.Read()
if err ! = nil {
    close(c.readCh)
    return
}

may be executed before recover SetReadDeadline, And just in time for the deadline causing read coroutines to exit.

thank you

@button-chen
Copy link
Author

Hello, can you take a look at this pr @worg

Copy link
Collaborator

@worg worg left a comment

Choose a reason for hiding this comment

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

can we add a couple of tests for this? specially covering the deadline from context

conn.go Outdated
response, err := reader.Read()
if err != nil {
return nil, err
}
if ok && isNetConn {
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this the same as the check on line 165? I think you meant to use !ok
also why SetReadDeadline and not SetDeadline?

Copy link
Collaborator

Choose a reason for hiding this comment

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

additionaly, can we move this one above so it's next to the other if

Copy link
Author

Choose a reason for hiding this comment

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

The problem to be solved here is that after the stomp client sends the stomp protocol connect message, the stomp server does not reply due to some reasons, such as: stomp server resources are insufficient stomp client message, and the stomp client will block forever. The use of SetReadDeadline here is correct because we want reader.Read() to time out instead of blocking forever, Calling SetReadDeadline again at line 173 with the argument time.Time{} restores net.Conn's read blocking behavior because we don't want it to affect subsequent data interactions. thank you

Copy link
Author

Choose a reason for hiding this comment

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

Similar writing can be found in the standard library http, src/net/http/server.go can find many similar implementation methods, such as go1.23.1 (src/net/http/server.go):
20241024-094716

@button-chen
Copy link
Author

can we add a couple of tests for this? specially covering the deadline from context

ok

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