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

Question about using redigo for that it does not support concurrently Receive and Close #1531

Closed
tony21177 opened this issue Aug 2, 2021 · 3 comments · Fixed by #1641
Closed
Assignees
Labels
bug Something isn't working as documented

Comments

@tony21177
Copy link

tony21177 commented Aug 2, 2021

Fleet version (Head to the "My account" page in the Fleet UI or run fleetctl version): 3.10.1

Fleet tier (Are you using the free (Fleet Core) or paid (Fleet Basic) tier?): free

User role (Which role are you assigned in Fleet? (Admin, Maintainer, or Observer)):

3 roles are introduced in Fleet 4.0.0. If you're using a prior version of Fleet or don't know your assigned role, please feel free to leave this item blank.


🧑‍💻  Expected behavior

As Redigo's documentation describe:

Connections support one concurrent caller to the Receive method and one concurrent caller to the Send and Flush methods. No other concurrency is supported including concurrent calls to the Do method.

For full concurrent access to Redis, use the thread-safe Pool to get, use and release a connection from within a goroutine. Connections returned from a Pool have the concurrency restrictions described in the previous paragraph.

https://pkg.go.dev/github.com/garyburd/redigo/redis#hdr-Concurrency

💥  Actual behavior

https://github.com/fleetdm/fleet/blob/main/server/pubsub/redis_query_results.go#L163
redis_query_results.go#L163
the goroutine use the redis connection to Receive

https://github.com/fleetdm/fleet/blob/main/server/pubsub/redis_query_results.go#L167
redis_query_results.go#L167
but another goroutine use the same connection to Close.

Doesn't it violate the redigo not supporting concurrency limitation?

@tony21177 tony21177 added the bug Something isn't working as documented label Aug 2, 2021
@tony21177
Copy link
Author

tony21177 commented Aug 3, 2021

Hi,
Because the connection is retrieved from the connection pool,so it assure the thread safety?
If so,I will close the issue

@tony21177
Copy link
Author

tony21177 commented Aug 3, 2021

profile001.pdf
Actually,My fleet server bumps into a redis max connections issue , and the attached file is the goroutine debug pdf
the detailed description as the other issue I report #1541

@tony21177 tony21177 changed the title Wrongly used redigo for that it does not support concurrently Receive and Close Question about using redigo for that it does not support concurrently Receive and Close Aug 3, 2021
@noahtalerman noahtalerman added the :troubleshooting We have responded and requested next steps/help label Aug 10, 2021
@noahtalerman noahtalerman assigned chiiph and unassigned zwass Aug 10, 2021
@chiiph
Copy link
Contributor

chiiph commented Aug 11, 2021

Hi @tony21177, the connection retrieving from the pool is thread safe. But what I believe that means is that the retrieving is thread safe, not the usage. Which is why it says "use and release a connection from within a goroutine".

Thank you for reporting this, I'll have a PR soon.

@mikermcneil mikermcneil removed the :troubleshooting We have responded and requested next steps/help label Aug 11, 2021
@chiiph chiiph linked a pull request Aug 11, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as documented
Development

Successfully merging a pull request may close this issue.

5 participants