thread Context into protocol and dataunit APIs #16
Conversation
ydnar
commented
Dec 4, 2023
ydnar
commented
- protocol/dataunit: add Context support for cancelation and timeouts
- protocol: add Context support to Client
- protocol, protocol/dataunit: thread Context into Server, add Responder types
- protocol/dataunit: remove separate Reader and Writer types
- protocol/dataunit: clarify that the Context passed to Serve governs reads and writes on the underlying connection
- protocol: improve docs
…eads and writes on the underlying connection
| var cfg config.Config | ||
| cfg.Join(opts...) | ||
| return connect(conn, cfg) | ||
| } |
There was a problem hiding this comment.
When would someone use Connect vs. Dial?
There was a problem hiding this comment.
Dial will likely go away, or exist only as a helper, which would remove the direct dependency on crypto/tls in this package.
There was a problem hiding this comment.
Maybe Dial is better as an example?
| // Read the initial <greeting> from the server. | ||
| data, err := conn.ReadDataUnit() | ||
| data, err := dataunit.Receive(ctx, conn) | ||
| if err != nil { | ||
| return c, nil, err | ||
| } | ||
| body, err := c.coder.umarshalXML(data) | ||
| return c, body, err |
There was a problem hiding this comment.
The protocol client seems like the perfect place to enforce the ordering of messages, asserting here that this is a greeting or returning an error instead.
protocol/server.go
Outdated
| // RespondEPP writes an EPP response to the client. It blocks until the message is written, | ||
| // Context is cancelled, or the underlying connection is closed. | ||
| // | ||
| // RespondEPP can be called from an arbriary goroutine, but must only be called once. |
There was a problem hiding this comment.
Similar question about the "once" aspect.
| // | ||
| // [RFC 5730]: https://datatracker.ietf.org/doc/rfc5730/ | ||
| type Server interface { | ||
| // ServeEPP provides an client EPP request and a mechanism to respond to the request. | ||
| // It blocks until a response is received or the underlying connection is closed. | ||
| // The returned [Writer] should only be used once. The returned Writer will always | ||
| // It blocks until a response is received, Context is canceled, or the underlying connection is closed. |
There was a problem hiding this comment.
Is this blocking not until a response is sent/returned to the client? I didn't think the server would wait for the client to read (beyond a timeout) from the server and respond to it.
| // If no schemas are provided, a set of reasonable defaults will be used. | ||
| func Serve(conn dataunit.Conn, greeting epp.Body, schemas ...schema.Schema) (Server, error) { | ||
| func Serve(ctx context.Context, conn io.ReadWriter, greeting epp.Body, schemas ...schema.Schema) (Server, error) { |
There was a problem hiding this comment.
Appreciate the documentation, but perhaps this should be renamed greetingCtx to be clearer about the scope.
There was a problem hiding this comment.
Sorry for the greetingCtx comment duplication. This is the top level location that passes down to protocol.Connect.
| // If no schemas are provided, a set of reasonable defaults will be used. | ||
| func Connect(conn dataunit.Conn, schemas ...schema.Schema) (Client, epp.Body, error) { | ||
| func Connect(ctx context.Context, conn io.ReadWriter, schemas ...schema.Schema) (Client, epp.Body, error) { |
There was a problem hiding this comment.
| func Connect(ctx context.Context, conn io.ReadWriter, schemas ...schema.Schema) (Client, epp.Body, error) { | |
| func Connect(greetingCtx context.Context, conn io.ReadWriter, schemas ...schema.Schema) (Client, epp.Body, error) { |
I wonder if there's a better name than Connect for this function since it is really only about receiving the greeting, which initializes the EPP protocol over whatever connection is already established in conn.
There was a problem hiding this comment.
It initializes the Client, which maintains the schemas, but to your point…does it need to carry that state?
| // If no schemas are provided, a set of reasonable defaults will be used. | ||
| func Serve(conn dataunit.Conn, greeting epp.Body, schemas ...schema.Schema) (Server, error) { | ||
| func Serve(ctx context.Context, conn io.ReadWriter, greeting epp.Body, schemas ...schema.Schema) (Server, error) { |
There was a problem hiding this comment.
Sorry for the greetingCtx comment duplication. This is the top level location that passes down to protocol.Connect.
