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

Fix panic caused by ETCD client timeout on Revoke() #409

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

gibertoni
Copy link

When revoking a lease, for example on app shutdown, there is a scenario on which ETCD client takes more than revokeTimeout to respond.

In this scenario, the timed action would trigger a defer close(c) and close the channel, but wouldnt cancel the Revoke operation. Therefore, as the sd.cli.Revoke(ctx, sd.leaseID) returns and then tries to write the result on the channel, panic is triggered:

panic: send on closed channel

The suggested approach to handle timeouts with ETCD client is to set the timeout into the context.

Note that this same issue could happen on renewLease() if grantLeaseTimeout is ever smaller than etcdDialTimeout. The default configuration is safe, so I am not changing this one.

…xes panic caused by prematurely closing channel before Revoke returns
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9841627666

Details

  • 0 of 12 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 61.974%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cluster/etcd_service_discovery.go 0 12 0.0%
Totals Coverage Status
Change from base Build 9668931564: 0.008%
Covered Lines: 4873
Relevant Lines: 7863

💛 - Coveralls

@gibertoni gibertoni changed the base branch from main to v2 August 9, 2024 16:33
@reinaldooli reinaldooli merged commit 50c6877 into topfreegames:v2 Aug 19, 2024
4 checks passed
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.

3 participants