Skip to content
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

[3.5] server: don't panic in readonly serializable txn #14178

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

lavacat
Copy link

@lavacat lavacat commented Jun 29, 2022

Problem: We pass grpc context down to applier in readonly serializable txn.
This context can be cancelled for example due to timeout.
This will trigger panic inside applyTxn

Solution: Only panic for transactions with write operations

fixes #14110
backported from main #14149

Signed-off-by: Bogdan Kanivets bkanivets@apple.com

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@lavacat
Copy link
Author

lavacat commented Jun 29, 2022

PR isn't merged yet, but even if solution changes, I think test is relevant

@@ -474,7 +475,17 @@ func (a *applierV3backend) Txn(ctx context.Context, rt *pb.TxnRequest) (*pb.TxnR
txn.End()
txn = a.s.KV().Write(trace)
}
a.applyTxn(ctx, txn, rt, txnPath, txnResp)
_, err := a.applyTxn(ctx, txn, rt, txnPath, txnResp)
if err != nil && isWrite {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err != nil && isWrite {
if err != nil {

@ahrtr
Copy link
Member

ahrtr commented Aug 13, 2022

Please update this PR per #14149

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as requiring sync with the 'main' variant of the PR.

@lavacat
Copy link
Author

lavacat commented Aug 30, 2022

Sorry about delay. Will update soon.

@lavacat lavacat force-pushed the release-3.5-txn-panic branch from ceda863 to 8ce41b2 Compare September 1, 2022 08:01
Problem: We pass grpc context down to applier in readonly serializable txn.
This context can be cancelled for example due to timeout.
This will trigger panic inside applyTxn

Solution: Only panic for transactions with write operations

fixes etcd-io#14110
main PR etcd-io#14149

Signed-off-by: Bogdan Kanivets <bkanivets@apple.com>
@lavacat lavacat force-pushed the release-3.5-txn-panic branch from 8ce41b2 to 204d883 Compare September 1, 2022 08:02
@lavacat
Copy link
Author

lavacat commented Sep 13, 2022

@ahrtr @ptabor FYI, this is ready to merge

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you @lavacat

Please drop a message next time when you finish all requested change.

@ahrtr
Copy link
Member

ahrtr commented Sep 13, 2022

This is just a backport PR, so merging...

Please add a changelog item if you haven't done it. @lavacat

@ahrtr ahrtr merged commit 6c26693 into etcd-io:release-3.5 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants