[ADDED] context.Context support #275
Conversation
context.go
Outdated
| // in bytes and request expecting a single response. | ||
| func (nc *Conn) RequestWithContext(ctx context.Context, subj string, data []byte) (*Msg, error) { | ||
| if ctx == nil { | ||
| panic("nil context") |
There was a problem hiding this comment.
In general I don't think we have panics in the client code. We should either error or ignore a nil context.
There was a problem hiding this comment.
I agree on that, so for now I have removed the checks to not introduce calling panic in the nats client.
Here I was borrowing from the http, net and other libraries where panic is being called...
- https://github.com/golang/go/blob/3792db518327c685da17ca6c6faa4e1d2da4c33c/src/net/dial.go#L316-L319
- https://github.com/golang/go/blob/094498c9a13cd711ed45a65b153393eb8ae1566b/src/net/http/request.go#L323-L334
- https://github.com/go-redis/redis/blob/f33571c93c72d3c4717a7b3c0bce95136496f772/redis_context.go#L28-L32
...though in other packages like sql package those checks aren't introduced either so switched to follow that style instead:
There was a problem hiding this comment.
@wallyqs You have removed the panic on nil context, but is it not going to panic any way when dereferencing it in the code? (unless context's method themselves do nothing if context is nil).
There was a problem hiding this comment.
yes if nil it would panic when calling ctx.Done() anyway as you mention
context.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| default: |
There was a problem hiding this comment.
should return statement be under default?
context.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| default: |
context.go
Outdated
| // context gets canceled. | ||
| func (s *Subscription) NextMsgWithContext(ctx context.Context) (*Msg, error) { | ||
| if ctx == nil { | ||
| panic("nil context") |
There was a problem hiding this comment.
Same here for panic in client code.
context.go
Outdated
| subject string, | ||
| v interface{}, | ||
| vPtr interface{}, | ||
| ) error { |
There was a problem hiding this comment.
Thanks, fixed formatting here...
context.go
Outdated
| vPtr interface{}, | ||
| ) error { | ||
| if ctx == nil { | ||
| panic("nil context") |
context.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: |
There was a problem hiding this comment.
Return err as part of default.
| t.Fatalf("Expected request with timeout context to fail: %s", err) | ||
| } | ||
|
|
||
| // Reported error is "context canceled" from Context package, |
There was a problem hiding this comment.
Cancelled vs canceled, I think both are valid, but we should be consistent in spelling.
There was a problem hiding this comment.
Thanks Derek, I changed throughout to use canceled which is the one used in the go repo:
| if _, ok := err.(timeoutError); ok { | ||
| t.Errorf("Expected to not have a timeout error") | ||
| } | ||
| expected = `context canceled` |
wallyqs
force-pushed
the
add-context-support
branch
2 times, most recently
from
1533a2c
to
a23e4a4
Compare
|
LGTM |
Add context aware APIs based on original ones Rename prefix from context tests Add context integration to encoded requests
Add processNextMsgDelivered and reuse in NextMsg
wallyqs
force-pushed
the
add-context-support
branch
from
a23e4a4
to
855d5bf
Compare
|
LGTM |
Adds
context.Contextsupport to the +Go1.7 clients implementing the APIs suggested at https://github.com/nats-io/go-nats/issues/221#issuecomment-288193653Fixes #221