-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat(network): configurable timeout of packet_listener_proxy #66
Conversation
network/packet_listener_proxy.go
Outdated
// | ||
// The timeout is the write idle timeout. This means that if there are no WriteTo operations on the UDP session created | ||
// by NewSession for the specified amount of time, the proxy will end this session. | ||
func NewPacketProxyFromPacketListenerWithTimeout(pl transport.PacketListener, timeout time.Duration) (PacketProxy, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is not very scalable. What if there are other parameters in the future?
Since it's not a required parameter, let's provide a setter instead, like we do for the shadowsocks prefix.
Require params -> constructor, optional params -> setters.
There's the option of exposing the field, but setters are more flexible, as it allows us to migrate the implementation in the future if needed.
You will need to expose the concrete type in the return of NewPacketProxyFromPacketListener()
.
Perhaps call it ListenerPacketProxy
and have no exposed fields? Then one can call ListenerPacketProxy.SetIdleTimeout(timeout)
.
Another option is to use the "Options" pattern, which is popular in Go, but it's more complicated. See
Functional Options Pattern for an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to point out is that the expiration logic could be implemented as a wrapper around the PacketListener instead, which would allow for a more composable design and to be used in other contexts.
The TimeoutPacketListener would create a timer for each connection that will close the connection when idle. The consumer should get a ErrClosed when that happens and react accordingly.
The connections would be wrapped in order to reset the timer on writes.
That would make packet connections a lot more like stream conns.
Up to you, perhaps something for another PR in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, now we can use the following code to create a new packet proxy with a specific timeout. The option name WithPacketListenerWriteIdleTimeout
is a little bit verbose because it is under network
package.
NewPacketProxyFromPacketListener(pl, WithPacketListenerWriteIdleTimeout(5 * time.Minute))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, but let's have only one function
This PR replaces the `go-tun2socks/core` with `outline-sdk/*` in the Intra project, and removes the dependency on `tunnel.Tunnel`, making the `intra` package a standalone Go package that can be copied to other locations, such as the [Intra repository](https://github.com/Jigsaw-Code/Intra). I kept the signatures of all exported interfaces the same. We can refactor them after we move the code to the Intra repository. Related PR: Jigsaw-Code/outline-sdk#66
No description provided.