-
Notifications
You must be signed in to change notification settings - Fork 60
fix: use wrapped context's err when <-ctx.Done() case is executed #463
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
base: master
Are you sure you want to change the base?
Conversation
5f9ce77
to
5362b47
Compare
5362b47
to
7aa279e
Compare
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.
Thank you for the patch! I have a few comments.
|
||
### Fixed | ||
|
||
- Use wrapped context err when <-ctx.Done() case is executed |
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.
Please, add a link to the issue in the changelog entry.
- Use wrapped context err when <-ctx.Done() case is executed | |
- Use wrapped context err when <-ctx.Done() case is executed (#457). |
|
||
### Changed | ||
|
||
### Fixed |
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.
Please, move it into Changed
or Added
section of the CHANGELOG.md. It does not fixes anything, but looks like an improvment.
// Instruct msgpack to pack this struct as array, so no custom packer | ||
// is needed. | ||
_msgpack struct{} `msgpack:",asArray"` //nolint: structcheck,unused | ||
_msgpack struct{} `msgpack:",asArray"` // nolint: structcheck,unused |
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.
Please, revert the unnecessary change.
// Concurrency: 32, | ||
// RateLimit: 4*1024, |
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.
Please, revert the unnecessary change.
} | ||
|
||
/////////////////// | ||
// ///////////////// |
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.
Please, revert the unnecessary change.
conn := test_helpers.ConnectWithValidation(t, dialer, opts) | ||
defer conn.Close() | ||
req := NewPingRequest().Context(nil) //nolint | ||
req := NewPingRequest().Context(nil) // nolint |
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.
Please, revert the unnecessary change.
Version: ProtocolVersion(1), | ||
Features: []iproto.Feature{iproto.IPROTO_FEATURE_STREAMS}, | ||
}).Context(nil) //nolint | ||
}).Context(nil) // nolint |
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.
Please, revert the unnecessary change.
Version: ProtocolVersion(1), | ||
Features: []iproto.Feature{iproto.IPROTO_FEATURE_STREAMS}, | ||
}).Context(ctx) //nolint | ||
}).Context(ctx) // nolint |
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.
Please, revert the unnecessary change.
fut.SetError(fmt.Errorf( | ||
"context is done (request ID %d): %w", | ||
fut.requestId, context.Cause(ctx), | ||
)) |
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.
Please, add a test that we could check the context error existence with errors.As/errors.Is
.
Related issues:
#457