-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Force concurrent LdapConnection in new process #41880
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
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
src/libraries/System.DirectoryServices.Protocols/tests/LdapConnectionTests.cs
Outdated
Show resolved
Hide resolved
dbdff59 to
bde6e65
Compare
I think I'm going to need to give up on getting this to repro in CI. It's very rare. I was able to get a repro in WSL locally. Rather than calling through S.DS.P I just put the ldap_init PInvoke directly in a test program and used the same method as this test and had it run for 10,000 iterations. I was able to hit one version of this crash: I'm trying a few times without a fix to make sure I can repro then will try with the fix to make sure it goes away. |
|
Yeah I was going to suggest doing PInvoke directly to at least confirm this is the cause, but having that regression test would have been nice. We do a bit if work on that constructor and run several operations before that call so I do believe it will be really har ld to get consistent repro going directly from S.DS.P unfortunately. |
9e96c91 to
7dbc4b1
Compare
|
So without the fix I hit this about 1 / 3000 times when directly invoking the PInvoke. After the fix I don't hit it at all in 10000 iterations. I'm running more to confirm I no longer repro. |
|
Probably want to minimize the diff for porting purposes |
I did specifically consider this. The reason for the larger diff was that this library did not create a class specific to libldap. Rather than put a static constructor on the Interop class (which could conflict with other usage) I moved all the libLdap Pinvokes to their own class. This change was done in a way that "if it compiles, it is correct" at least from the rename perspective. I could make a smaller change in release that adds the static constructor to the "Interop" class. at the moment this library only uses libldap, if you think a smaller diff is worth having the potential maintenance issue in the future. |
OpenLDAP requires a single call for initialization before any other concurrent call. This fixes asserts and segfaults we were seeing when calling OpenLDAP concurrently.
7dbc4b1 to
c1a9d95
Compare
|
Ah - I see. Makes sense. |
src/libraries/System.DirectoryServices.Protocols/tests/LdapConnectionTests.cs
Outdated
Show resolved
Hide resolved
...ies/System.DirectoryServices.Protocols/tests/System.DirectoryServices.Protocols.Tests.csproj
Outdated
Show resolved
Hide resolved
|
While I'm fine with the change proposed here, another option if we want to minimize the changes and still not add a static constructor to Interop class would be to move the static constructor one level up to LdapConnection class right? This is the primary one that interacts with libldap anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but looks good! Thanks so much for fixing!
|
So 150K iterations after fix and no-repro after seeing about once every 3K before locally. I think this fixed it. Will merge in master and open RC2 port. Will test more in RC2. |
|
/backport to release/5.0-rc2 |
|
Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448 |
Fix #39009
Ensure an initial non-concurrent call to OpenLDAP
OpenLDAP requires a single call for initialization before any other concurrent call.
This fixes asserts and segfaults we were seeing when calling OpenLDAP concurrently.