CBG-3834-prereq Add cancellable context to blip.Context #77
Conversation
There was a problem hiding this comment.
I left some comments thinking this wasn't a WIP, sorry about the obvious ones.
I like the idea of passing a generic context.Context into a blip context, even if you call it a cancelCtx right now. Having this context be available would allow a future enhancement to include context information. I suppose that this is already done by https://github.com/couchbase/sync_gateway/blob/9ce7f7bdea12048db13d6a5b694c357cbc4a9e1f/db/blip.go#L62 so maybe this is unnecessary.
|
|
||
| // HTTP Handler wrapping websocket server | ||
| http.Handle("/TestServerContextClose", server) | ||
| listener, err := net.Listen("tcp", ":0") |
There was a problem hiding this comment.
I think this needs to be closed in a defer, or it will leak?
context_test.go
Outdated
| assert.True(t, sent) | ||
|
|
||
| // Read the echo response | ||
| response := echoRequest.Response() // <--- SG #3268 was causing this to block indefinitely |
There was a problem hiding this comment.
I'm not sure what this comment is referring to with SG #3268?
context_test.go
Outdated
| // - Actual: the echo client blocks indefinitely trying to read the response | ||
| func TestServerContextClose(t *testing.T) { | ||
|
|
||
| blipContextEchoServer, err := NewContext(defaultContextOptions) |
There was a problem hiding this comment.
As an API design, I think NewContext should look like func NewContext(ctx context.Context, opts ContextOptions) (*Context, error)
which will set the cancelCtx on blip.Context and force the callers to specify it, even if they decide to do context.TODO() or context.Background().
context.go
Outdated
| @@ -133,6 +135,14 @@ func (blipCtx *Context) GetBytesReceived() uint64 { | |||
| return blipCtx.bytesReceived.Load() | |||
| } | |||
|
|
|||
| // Opens a BLIP connection to a host. | |||
There was a problem hiding this comment.
This comment doesn't make sense, but see below where I think we should force passing a context when NewContext is specified. I think we could fail when you call BlipContext.Dial.
adamcfraser
force-pushed
the
CBG-3834-prereq
branch
from
627ad66 to
a3e9229
Compare
Allows callers to specify an optional cancellable context on a blipContext disconnect any connected blip clients when that context is used to create a WebSocketServer.
context.go
Outdated
| @@ -133,6 +135,18 @@ func (blipCtx *Context) GetBytesReceived() uint64 { | |||
| return blipCtx.bytesReceived.Load() | |||
| } | |||
|
|
|||
| // GetCancelCtx returns cancelc=Ctx if it has been set. Otherwise returns non-cancellable context. | |||
There was a problem hiding this comment.
| // GetCancelCtx returns cancelc=Ctx if it has been set. Otherwise returns non-cancellable context. | |
| // GetCancelCtx returns a cancellation if it has been set by SetCancelCtx. Otherwise returns non-cancellable context. |
context.go
Outdated
| return context.TODO() | ||
| } | ||
|
|
||
| func (blipCtx *Context) SetCancelCtx(cancelCtx context.Context) { |
There was a problem hiding this comment.
| func (blipCtx *Context) SetCancelCtx(cancelCtx context.Context) { | |
| // SetCancelCtx will set a cancellation context for websocket reading. Cancelling this context will close the websocket connection with an error code. This needs to be called before Context.Dial or Context.DialConfig is run, and will have no effect after. | |
| func (blipCtx *Context) SetCancelCtx(cancelCtx context.Context) { |
I don't know actually know the right error to document here, but I think it'd be helpful to document that.
Given that this set has no effect if you call after Dial or DialConfig are you sure you don't want to add add this to ContextOptions to be sure that the caller has to specify it at the right time? I won't ask again, and this is your call.
adamcfraser
force-pushed
the
CBG-3834-prereq
branch
from
67df25c to
cc8d8eb
Compare
Allows callers to specify an optional cancellable context on a blipContext disconnect any connected blip clients when that context is used to create a WebSocketServer.