Skip to content

ZOOKEEPER-4299: Use lock free subroutines for clist in zoo_amulti since it is not multithreaded #2256

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Gowrima
Copy link
Contributor

@Gowrima Gowrima commented May 5, 2025

No description provided.

/* initialize mutex and condition variable in clist */
clist.head = NULL;
clist.last = NULL;
#ifdef THREADED
Copy link
Member

Choose a reason for hiding this comment

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

I checked code related to zoo_amulti and did not find concurrent usage of this list. The list is constructed in zoo_amulti and deconstructed in deserialize_multi. I would recommend to use queue_completion_nolock and dequeue_completion_nolock in these two places. Regarding the fix, the inited mutex/cond will leak as they are not destroyed.

@Gowrima Gowrima closed this May 13, 2025
@Gowrima Gowrima deleted the ZOOKEEPER-4299 branch May 13, 2025 19:02
@Gowrima Gowrima restored the ZOOKEEPER-4299 branch May 13, 2025 19:47
@Gowrima Gowrima reopened this May 13, 2025
@Gowrima
Copy link
Contributor Author

Gowrima commented May 13, 2025

@kezhuw https://github.com/apache/zookeeper/blob/master/zookeeper-client/zookeeper-client-c/src/mt_adaptor.c#L63
https://github.com/apache/zookeeper/blob/aeadf574c0463e7e3db092b588033d8347989d4c/zookeeper-client/zookeeper-client-c/src/zookeeper.c#L3142

Please take a look at this code. Mutex lock is already being called without proper initialization. This will lead to EINVAL error at some point in the future. Do you recommend restructuring this code to remove the mutex and condition variable from completion_head_t?

@kezhuw
Copy link
Member

kezhuw commented May 14, 2025

Do you recommend restructuring this code to remove the mutex and condition variable from completion_head_t?

No.

I checked code related to zoo_amulti and did not find concurrent usage of this list. The list is constructed in zoo_amulti and deconstructed in deserialize_multi. I would recommend to use queue_completion_nolock and dequeue_completion_nolock in these two places.

This is what I recommend. To be specific, I mean those two places:

queue_completion(&clist, entry, 0);
,
completion_list_t *entry = dequeue_completion(clist);

@Gowrima Gowrima changed the title ZOOKEEPER-4299: Initialize mutex and condition variable within clist head ZOOKEEPER-4299: Use lock free subroutines for clist in zoo_amulti since it is not multithreaded May 15, 2025
@Gowrima
Copy link
Contributor Author

Gowrima commented May 15, 2025

@kezhuw there is no dequeue_completion_nolock().

@kezhuw
Copy link
Member

kezhuw commented May 16, 2025

@Gowrima You could write one. queue_completion is an good example for rewriting.

@kezhuw
Copy link
Member

kezhuw commented May 16, 2025

cleanup_failed_multi should also be updated.

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