Passes basic tests, I will load/stress test this tomorrow.

Could you add the "invalid state" to the log message?

Why did you remove the pdumutex condition test? While it's unlikely it would fail, it doesn't hurt to keep the check.

java-style comment...

Could you add the "invalid state" to the log message?

Never mind, I see there are only two possible states :-)

Anyway, some minor comments but the rest looks good, ack.

Minor point. Do you mind to set c->c_state = CONN_STATE_INIT only after pthread_mutex_init call.

@mreynolds 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 :)

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.
But with NS we can imagine parallel handler, that access the connection table in parallel. Correct ?
Is that part of code robust for parallel new connection handers ?
IMHO the ct->table_mutex should be acquired for the all functions.

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.

@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?

@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 (https://pagure.io/389-ds-base/issue/50489) as the NS ticket/PR (#49569 / https://pagure.io/389-ds-base/pull-request/49636#) is more focus on solving the double lock (c_mutex, job.monitor) deadlock than concurrent access to the connection table.

It looks like the addition of the c_state change is highlighting a potential connection allocation issue under NS, patch to come shortly ...

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3542

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.