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

Dialer's timeout reset #128

Closed
jakubdal opened this issue Jul 12, 2017 · 3 comments
Closed

Dialer's timeout reset #128

jakubdal opened this issue Jul 12, 2017 · 3 comments

Comments

@jakubdal
Copy link

Hello,

I've encountered a following (i think) bug:
I'm creating a client using DialWithDialerTLS. Dialer has a set timeout. The timeout is set on connection.
After that, I'm executing a Login command.
In Login, because my client doesn't have timeout set on itself, the connection's Deadline is reset, and so the command blocks, even if Dialer's timeout is exceeded.

I do believe if it's not a bug, it's at least confusing, because of a comment saying // Among other uses, this allows us to apply a connection timeout. on DialWithDialerTLS.

I think about two solutions:

  • Set Dialer's timeout to Client
  • Remove the comment, and possibly add an information to documentation about this case

Looking forward to your reply :)

@emersion
Copy link
Owner

Dialer timeouts only applies when the connection is being established: https://golang.org/pkg/net/#Dialer.Timeout

After that, the Dialer timeout has no effect anymore. You can set a connection timeout with Client.Timeout: https://godoc.org/github.com/emersion/go-imap/client#Client.Timeout

@jakubdal
Copy link
Author

Yes, I am aware of that.

However that comment on DialWithDialerTLS got me misguided, and so I've assumed that the timeout propagates to the entire connection. I think that at least changing the comment should be enough. The bug itself was not obvious for me (maybe it should?), and so it took me a while to debug.

@emersion
Copy link
Owner

emersion commented Jul 12, 2017

You're right, the wording is incorrect. I changed that in a81f6b0.

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

No branches or pull requests

2 participants