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

Use tokio streams instead of crossbeam to fix deadlock issues #26

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

DaanDD
Copy link
Contributor

@DaanDD DaanDD commented Jul 5, 2023

I had issues with this package causing my whole application to hang. After some debugging I found it was because of the usage of crossbeam within an async environment. I created a PR to switch to tokio streams instead which fixes this issue. There's no need to do try_recv anymore in an infinite loop, instead the blocking recv().await call is used. I opted to use broadcast because it allows for client.send to remain non async. This meant I had to make Record clonable.

@johnmanjiro13
Copy link
Owner

@DaanDD
Thank you for fixing bug! I haven't realized this issue.
Could you merge main branch because I fixed the test workflow?

@johnmanjiro13 johnmanjiro13 added bug Something isn't working minor labels Jul 9, 2023
@DaanDD
Copy link
Contributor Author

DaanDD commented Jul 13, 2023

@johnmanjiro13 Done!

@johnmanjiro13
Copy link
Owner

@DaanDD
I merged #25 and this pr is conflicted with main.
Please resolve it 🙏

@DaanDD
Copy link
Contributor Author

DaanDD commented Jul 17, 2023

@johnmanjiro13 Fixed the merge conflict

Copy link
Owner

@johnmanjiro13 johnmanjiro13 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.

@@ -17,15 +17,13 @@ include = [
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
async-trait = "0.1.61"
Copy link
Owner

Choose a reason for hiding this comment

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

I'm glad to remove these dependencies 👍

@johnmanjiro13 johnmanjiro13 merged commit e5c21ee into johnmanjiro13:main Jul 18, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Jul 18, 2023
@johnmanjiro13
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants