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 pubsub redis close bug #769

Merged
merged 1 commit into from
Mar 20, 2021

Conversation

Taction
Copy link
Member

@Taction Taction commented Mar 19, 2021

Description

Redis component close function called r.client.Close() .
The pollNewMessagesLoop loop run in a goroutine use client to read from redis which will lead to an error: error retrieving pending Redis messages: redis: client is closed.
When err occured this loop will continue. The context closed check will never have chance to run.

if err != nil {
	r.logger.Errorf("redis streams: error reading from stream %s: %s", stream, err)
	continue
}
// below will have no chance to run if client is closed

We need move the check to the top of this loop.

Issue reference

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

When component close function called `r.cancel()` and `r.client.Close()` will be excuted.
The pollNewMessagesLoop loop use client to read from redis which will lead to err. And when err occured this loop will continue.
We need to check context on the top of this loop.
@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #769 (100e41c) into master (1924618) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
+ Coverage   32.97%   33.02%   +0.04%     
==========================================
  Files          96       96              
  Lines        8549     8549              
==========================================
+ Hits         2819     2823       +4     
+ Misses       5473     5467       -6     
- Partials      257      259       +2     
Impacted Files Coverage Δ
pubsub/redis/redis.go 22.40% <0.00%> (ø)
state/cloudstate/cloudstate_crdt.go 10.37% <0.00%> (+1.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1924618...100e41c. Read the comment docs.

Copy link
Member

@pkedy pkedy left a comment

Choose a reason for hiding this comment

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

Yep! Totally makes sense!

@artursouza artursouza added this to the v1.1 milestone Mar 20, 2021
@artursouza artursouza merged commit fb09a07 into dapr:master Mar 20, 2021
@Taction Taction deleted the pubsub_redis_close_bugfix branch April 21, 2021 03:49
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