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 context-aware method to enable timeout + cancel for connections #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nilslice
Copy link
Owner

@practice-golang, I couldn't push to your branch, so I've pulled your changes into a branch here. Some minor modifications have been made, but I'd like you to verify (if possible) that the sending still works as expected.

Each context should be passed in per call to SendWithContext so that a new context is used per dial, rather than using a global context at the package level. This isn't a super obvious pattern, and I'm sure your solution would have been ok in most cases! This is just a safer way to guarantee that no goroutines share the same context and end up colliding cancellations or timeouts.

If you can, please test in the sending environment you have been working in and let me know if there are any issues. Thanks for your help!

@nilslice nilslice mentioned this pull request Nov 11, 2018
@practice-golang
Copy link

Hello @nilslice.
Ok, I will try it. 😄

@practice-golang
Copy link

practice-golang commented Nov 12, 2018

Because I don't know how to edit codes in other branch, I wrote the codes here.

Example in README.md - If cancel is not needed, _ under-score is ok.

ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
defer cancel()

email.go : 129 - Text in email was not sent.

_, err = msg.Write([]byte("DATA\r\n\r\n" + m.Body + "\r\n"))
// _, err = fmt.Fprint(msg, m.Body)
if err != nil {
	return err
}

Then, it works well.

Thank you @nilslice. 😄

EDIT: time package should be imported in example.

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.

None yet

2 participants