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

Implement mutex on rabbitmq-event to control connection #2380

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

david-goncalves
Copy link
Contributor

After implementing a new thread to handle the heartbeat, we notice some SIGABRT on amqp_simple_wait_frame_noblock occasionally.
To address the issue, we open the discussion on the librabbitmq-c community and, they said that we shouldn't access the same channel in different threads without controlling it.
Based on other Janus models, we tried to implement janus_mutex.
We are testing these changes for a couple of days without having coredumps.

@lminiero
Copy link
Member

lminiero commented Oct 5, 2020

Apologies for the late reply, I must have missed the notification about this PR. I'm a bit hesitant any time I see a new mutex. Have you ensured that the way core and transport interact this can never lead to a deadlock?

@david-goncalves
Copy link
Contributor Author

The mutex is only applied to wrap the access to rabbitmq functions on Event Handler so, we think the core isn't affected by the modification made.
At this point, if we try to publish an event to the RabbitMQ and check if it's running at the same time (work done on different threads), we receive a SIGABRT.

@lminiero
Copy link
Member

lminiero commented Oct 8, 2020

Do you think the RabbitMQ transport plugin may be impacted as well?

@david-goncalves
Copy link
Contributor Author

Since transport has it's own channel I don't think this change could affect it. The mutex is only used here to guarantee that both threads on rabbitmq event handler (thread to send events and to check rabbitmq status) don't try to use the channel at same time.

@lminiero
Copy link
Member

lminiero commented Oct 9, 2020

No, what I meant is, do you think the transport plugin has the same issue that your patch fixes for the event handler?

@david-goncalves
Copy link
Contributor Author

I never used the transport rabbitmq but it may have the same issue since rmq_client->rmq_conn is accessed on both threads. The best is to test it out to see if the problem can be triggered on this module also. I could try it.

@lminiero
Copy link
Member

@david-goncalves if you could check that would help, thanks! In the meanwhile I'll merge your patch for the event handler, thanks again 👍

@lminiero lminiero merged commit 42ac7dd into meetecho:master Oct 13, 2020
PauKerr pushed a commit to caffeinetv/janus-gateway that referenced this pull request Nov 11, 2020
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.

2 participants