Skip to content

Add SafeDial to block connections #270

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 9, 2021
Merged

Add SafeDial to block connections #270

merged 2 commits into from
Dec 9, 2021

Conversation

mrdg
Copy link
Contributor

@mrdg mrdg commented Dec 8, 2021

Closes #255

@mrdg mrdg added the type: chore work needed to keep the product and development running smoothly label Dec 8, 2021
@mrdg mrdg requested a review from a team as a code owner December 8, 2021 14:16
mraerino
mraerino reviewed Dec 8, 2021
View reviewed changes
Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

could we instead alter SafeRoundtripper and SafeHTTPClient to use the SafeDial function? this would allow continued use and is probably a good abstraction right?

@mrdg
Copy link
Contributor Author

mrdg commented Dec 8, 2021

could we instead alter SafeRoundtripper and SafeHTTPClient to use the SafeDial function? this would allow continued use and is probably a good abstraction right?

SafeRoundtripper takes an http.RoundTripper so you can't configure the dial function on that like with http.Transport

@mraerino
Copy link
Contributor

mraerino commented Dec 8, 2021

maybe offer a safe transport abstraction?

@mrdg
Copy link
Contributor Author

mrdg commented Dec 8, 2021

maybe offer a safe transport abstraction?

Added SafeTransport. I kept SafeDial exported in case callers need to set custom dialer settings.

mraerino
mraerino approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

nice!

@mrdg mrdg merged commit 6386f12 into master Dec 9, 2021
@mrdg mrdg deleted the safe-dial branch December 9, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mraerino mraerino mraerino approved these changes

Assignees
No one assigned
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SafeRoundTripper can leak goroutines
2 participants
@mrdg @mraerino