PR - Ticket 50459 - c_mutex to use pthread_mutex to allow ns sharing #3542
Comments
|
Comment from firstyear (@Firstyear) at 2019-07-08 07:46:29 Passes basic tests, I will load/stress test this tomorrow. |
|
Comment from mreynolds (@mreynolds389) at 2019-07-08 20:27:53 Could you add the "invalid state" to the log message? |
|
Comment from mreynolds (@mreynolds389) at 2019-07-08 20:29:45 Why did you remove the pdumutex condition test? While it's unlikely it would fail, it doesn't hurt to keep the check. |
|
Comment from mreynolds (@mreynolds389) at 2019-07-08 20:33:41 java-style comment... |
|
Comment from mreynolds (@mreynolds389) at 2019-07-08 20:40:54
Never mind, I see there are only two possible states :-) Anyway, some minor comments but the rest looks good, ack. |
|
Comment from tbordaz (@tbordaz) at 2019-07-09 11:58:00 Minor point. Do you mind to set c->c_state = CONN_STATE_INIT only after pthread_mutex_init call. |
|
Comment from firstyear (@Firstyear) at 2019-07-10 02:10:22 @mreynolds389 Thanks for the detailed check, I'll fix these up now. I think you maybe misread the diff, I left the pdumutex check inplace? I just removed the "c_mutex || pdumutex" and changed it to "pdumutex" only? @tbordaz CONN_STATE_INIT must be the first thing we set to indicate we have started to change the structure, meaning that after this point it could be in a partial init state, requiring free. But not that it even matters, this is C, so the compiler can and will re-arrange all of these assignments based on what's fast, up until we init/lock a mutex. So really, no matter how we arrange this, the compiler will find a way to surprise you ;) I'll fix the comment and log message now :) |
|
Comment from firstyear (@Firstyear) at 2019-07-10 02:58:02 1 new commit added
|
|
Comment from tbordaz (@tbordaz) at 2019-07-10 10:35:54 Sorry to be nit picker but I still have a concern. Currently there is no real issue because the new connection handler is a single thread. Setting c_state=INIT before the real allocation of mutex means that an handler can return this connection while it is not yet fully initialized. |
|
Comment from firstyear (@Firstyear) at 2019-07-11 02:44:43 @tbordaz I think that's a concern for the NS ticket, not this one. This is just the change to pthread mutex, and I would like this merged as a single, simple unit of change. We can worry about the conntable management in the NS ticket which already has a number of these concurrency concerns. Is that reasonable for me to acknowledge your (very valid and thoughtful!) concern in the ns work ticket instead? |
|
Comment from tbordaz (@tbordaz) at 2019-07-11 10:05:25 @Firstyear, you are right. This concern existed before your patch. The patch looks very good. ACK. Regarding the concern, I preferred to open a separated ticket (#3546) as the NS ticket/PR (49569 / #2695#) is more focus on solving the double lock (c_mutex, job.monitor) deadlock than concurrent access to the connection table. |
|
Comment from firstyear (@Firstyear) at 2019-07-12 03:10:04 rebased onto d622686 |
|
Comment from firstyear (@Firstyear) at 2019-07-12 03:10:43 Pull-Request has been merged by Firstyear |
|
Comment from firstyear (@Firstyear) at 2019-07-12 05:35:56 It looks like the addition of the c_state change is highlighting a potential connection allocation issue under NS, patch to come shortly ... |
|
Patch |
Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50485
Bug Description: To allow nunc-stans to share the same lock as c_mutex
we need to change conn to use a pthread_mutex instead.
Fix Description: Change c_mutex to pthread
Resolves: #3517
Author: William Brown william@blackhats.net.au
Review by: ???
The text was updated successfully, but these errors were encountered: