Skip to content

Conversation

Maximilan4
Copy link

Related issues:
#457

@Maximilan4 Maximilan4 force-pushed the patch/457.ctxerr branch 3 times, most recently from 5f9ce77 to 5362b47 Compare September 29, 2025 19:25
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a 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
Copy link
Collaborator

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.

Suggested change
- Use wrapped context err when <-ctx.Done() case is executed
- Use wrapped context err when <-ctx.Done() case is executed (#457).


### Changed

### Fixed
Copy link
Collaborator

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
Copy link
Collaborator

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.

Comment on lines +92 to +93
// Concurrency: 32,
// RateLimit: 4*1024,
Copy link
Collaborator

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.

}

///////////////////
// /////////////////
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Comment on lines +987 to +990
fut.SetError(fmt.Errorf(
"context is done (request ID %d): %w",
fut.requestId, context.Cause(ctx),
))
Copy link
Collaborator

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.

@oleg-jukovec oleg-jukovec requested a review from bigbes September 30, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants