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

Channel and thread safety #84

Open
pgouv opened this issue Jun 15, 2023 · 5 comments
Open

Channel and thread safety #84

pgouv opened this issue Jun 15, 2023 · 5 comments

Comments

@pgouv
Copy link

pgouv commented Jun 15, 2023

I was looking at SendAndWaitResponseProducer.
It seems that it is designed to be thread safe. However since bunny/rabbitmq channel is shared is it really safe?
According to bunny documentation channels must not be shared. Is it a special case?

@indrekj
Copy link
Member

indrekj commented Jun 16, 2023

Hey. It's been a while since I last touched this part of the codebase. IIRC we were thinking about thread safety. We've been running this for years (hundreds of messages per second per instance) without any thread safety problems. I'm relatively sure that regular YARV ruby is fine. The other ruby variants may not though.

@pgouv
Copy link
Author

pgouv commented Jun 16, 2023

Do you run this in a multi-threaded server such as Puma? Servers like passenger shouldn't have a problem though.
A possible solution could be to use ConnectionPool gem to create a pool of channels for publishing. This gem is used by Rails too. However there could be cases where channel is closed by the server and you wont have much control but this could happen to current implementation too.
Probably it might be an edge case.

@indrekj
Copy link
Member

indrekj commented Jun 16, 2023

Yes, we're using puma with multiple threads.

I think before starting to rewrite this, we should first try to produce the issue. I don't want to start changing anything if there's no issue.

@indrekj
Copy link
Member

indrekj commented Jun 16, 2023

Btw, if you're just starting then our experience says that using regular HTTP is better than deliver_with_response for RPC. We still use deliver and tap_into, but try to avoid deliver_with_response. Mainly because HTTP provides more control and is more standard for RPC use cases.

@pgouv
Copy link
Author

pgouv commented Jun 16, 2023

Yes, we're using puma with multiple threads.

I think before starting to rewrite this, we should first try to produce the issue. I don't want to start changing anything if there's no issue.

Probably a stress test would help. I may try to test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants