Skip to content

api: introduce net.Listener and net.Conn wrappers #8

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

Merged
merged 11 commits into from
Jun 15, 2019
Merged

Conversation

pires
Copy link
Owner

@pires pires commented Jun 15, 2019

This is an attempt to partially address #3 with pieces of #2. The reason why I'm not merging #2 (or the code I have here) is because I don't feel confident enough it isn't introducing breaking changes to any existing users of this library who may be pulling from master branch.

@Freeaqingme @mgouline @elico @anisse can you PTAL?

TODO

@coveralls
Copy link

coveralls commented Jun 15, 2019

Coverage Status

Coverage decreased (-2.9%) to 86.902% when pulling 265cd46 on pires/pr_2_rework into 9c0bafa on master.

@pires pires force-pushed the pires/pr_2_rework branch 2 times, most recently from cdc20e9 to adee7ed Compare June 15, 2019 15:31
@pires pires force-pushed the pires/pr_2_rework branch from 506785b to c0d329c Compare June 15, 2019 16:07
@pires pires merged commit 2c19fd5 into master Jun 15, 2019
@pires pires deleted the pires/pr_2_rework branch June 15, 2019 16:34
@anisse
Copy link

anisse commented Jun 16, 2019

LGTM, especially with the updated comments from the later commits.

It seems this implementation is better than my own wrapper, which is a bit more naive (https://github.com/anisse/proxylistener/blob/master/proxylistener.go) :

  • initially I really liked the idea of wrapping net.Listen() directly as I did, but I realize now it's much better to wrap a listener, which is an interface so it can be something else than what stdlib provides. I also allows passing a tuned tcplistener (with setdeadline for example)
  • In addition, it gives you better control with the additional read timeout, which allows having different behaviour than just the accept timeout (which my implementation does not allow anyway)
  • I also didn't really like the use of sync.Once, but I see now that it makes sense in the context of having a Conn that can be used in the client and server case (mine only does the server case).

In conclusion, LGTM 👍

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

Successfully merging this pull request may close these issues.

An example for a Dialer and Listener is required.
4 participants