Force concurrent LdapConnection in new process

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448

[ericstj]

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.

[Dotnet-GitSync-Bot]

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.

[ericstj]

All checks have passed

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:

dotnet: ../nptl/pthread_mutex_lock.c:79: __pthread_mutex_lock: Assertion `mutex->__data.__owner == 0' failed.
./test.sh: line 1: 18048 Aborted (core dumped) ( dotnet /mnt/c/scratch/ds.p.linux/bin/Debug/netcoreapp3.1/ds.p.linux.dll )

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.

[joperezr]

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.

[ericstj]

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.

[danmoseley]

Probably want to minimize the diff for porting purposes

[ericstj]

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.

[danmoseley]

Ah - I see. Makes sense.

[joperezr]

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.

[joperezr]

Left a few comments but looks good! Thanks so much for fixing!

[ericstj]

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.

[ericstj]

/backport to release/5.0-rc2

[github-actions]

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/243980448