pkg/transport, rafthttp: CancelableTransport #7602
Conversation
pkg/transport/transport.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return conn, nil |
There was a problem hiding this comment.
why isn't return dialer.DialContext(ctx, "unix", addr) sufficient for line 83-87?
There was a problem hiding this comment.
there was some other stuff here, will clean up
| func (c *CancelableTransport) Cancel() { c.cancel() } | ||
|
|
||
| func (c *CancelableTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| if req.Context() != context.Background() { |
There was a problem hiding this comment.
how should we handle the case that req.Context() == context.TODO()?
There was a problem hiding this comment.
It's checking for the default for a request, which is context.Background(). The request would have to explicitly override it with TODO
There was a problem hiding this comment.
If the context of the request is overridden with TODO, then that request can't be cancelled even if CancelableTransport.Cancel() is called right? If yes, is that an expected behavior?
There was a problem hiding this comment.
It's meant to override the default ctx. If the user passes their own ctx into it, this won't override it.
There was a problem hiding this comment.
got it. I think adding a comment about this check will be great.
| if ctx.Err() != nil { | ||
| return nil, ctx.Err() | ||
| } | ||
| return tdialer.DialContext(ctx, net, addr) |
There was a problem hiding this comment.
do you know if DialContext() handle cancelled/erred ctx properly? so that you don't have to check the ctx.Err() != nil beforehand.
| "net" | ||
| "net/http" | ||
| "strings" | ||
| "time" | ||
| ) | ||
|
|
||
| type CancelableTransport struct { |
There was a problem hiding this comment.
should we have some sort of test for CancelableTransport?
heyitsanthony
force-pushed
the
cancelable-transport
branch
from
74c92a4
to
0656989
Compare
| errc := make(chan error, 1) | ||
| go func() { | ||
| defer close(errc) | ||
| req, reqerr := http.NewRequest("GET", sock, strings.NewReader("abc")) |
There was a problem hiding this comment.
Should the sock here be prepend with unix:, e.g unix:whatever:123? Or else tr.RoundTrip(req) will never connect to the listener created by NewUnixListener(sock)?
heyitsanthony
force-pushed
the
cancelable-transport
branch
from
0656989
to
ae4e6f5
Compare
|
lgtm, thanks! |
pkg/transport/transport.go
Outdated
| } | ||
|
|
||
| func (c *CancelableTransport) Ctx() context.Context { return c.ctx } | ||
| func (c *CancelableTransport) Cancel() { c.cancel() } |
| cancel context.CancelFunc | ||
| } | ||
|
|
||
| func (c *CancelableTransport) Ctx() context.Context { return c.ctx } |
| func (c *CancelableTransport) Ctx() context.Context { return c.ctx } | ||
| func (c *CancelableTransport) Cancel() { c.cancel() } | ||
|
|
||
| func (c *CancelableTransport) RoundTrip(req *http.Request) (*http.Response, error) { |
|
lgtm |
A transport that is tied to a context. Lets the consumer cancel all requests and dials with a single Cancel.
heyitsanthony
force-pushed
the
cancelable-transport
branch
from
ae4e6f5
to
61f8d61
Compare
gyuho
force-pushed
the
master
branch
2 times, most recently
from
44ca396
to
4301f49
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
A transport that is tied to a context. Lets the consumer cancel all requests
and dials with a single Cancel.
Related: #7594