From 1f55a4aa3688344bed081a7d7bba6f101b65f961 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 11 Jun 2020 15:42:52 -0700 Subject: [PATCH 1/5] Fix BeginSendRequest and EndSendRequest on LdapConnection --- .../Protocols/ldap/LdapConnection.cs | 94 +++++++++++-------- .../ldap/LdapPartialResultsProcessor.cs | 5 +- .../tests/DirectoryServicesProtocolsTests.cs | 12 ++- 3 files changed, 72 insertions(+), 39 deletions(-) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs index c66383e6d8a97f..5b4ae9aa8db735 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs @@ -2,17 +2,16 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System.Globalization; -using System.Net; using System.Collections; using System.ComponentModel; -using System.Text; using System.Diagnostics; +using System.Net; using System.Runtime.InteropServices; -using System.Xml; -using System.Threading; using System.Security.Cryptography.X509Certificates; -using System.Diagnostics.CodeAnalysis; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using System.Xml; namespace System.DirectoryServices.Protocols { @@ -44,7 +43,6 @@ internal enum LdapResult private bool _needRebind = false; internal static Hashtable s_handleTable = null; internal static object s_objectLock = null; - private readonly GetLdapResponseCallback _fd = null; private static readonly Hashtable s_asyncResultTable = null; private static readonly LdapPartialResultsProcessor s_partialResultsProcessor = null; private static readonly ManualResetEvent s_waitHandle = null; @@ -84,7 +82,6 @@ public LdapConnection(LdapDirectoryIdentifier identifier, NetworkCredential cred public LdapConnection(LdapDirectoryIdentifier identifier, NetworkCredential credential, AuthType authType) { - _fd = new GetLdapResponseCallback(ConstructResponse); _directoryIdentifier = identifier; _directoryCredential = (credential != null) ? new NetworkCredential(credential.UserName, credential.Password, credential.Domain) : null; @@ -288,7 +285,9 @@ public DirectoryResponse SendRequest(DirectoryRequest request, TimeSpan requestT if (error == 0 && messageID != -1) { - return ConstructResponse(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true); + ValueTask vt = ConstructResponseAsync(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true, sync: true); + Debug.Assert(vt.IsCompleted); + return vt.GetAwaiter().GetResult(); } else { @@ -379,7 +378,30 @@ public IAsyncResult BeginSendRequest(DirectoryRequest request, TimeSpan requestT s_asyncResultTable.Add(asyncResult, messageID); - _fd.BeginInvoke(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true, new AsyncCallback(ResponseCallback), requestState); + ResponseCallback(ConstructResponseAsync(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true, sync: false), requestState); + + static async void ResponseCallback(ValueTask vt, LdapRequestState requestState) + { + try + { + DirectoryResponse response = await vt.ConfigureAwait(false); + requestState._response = response; + } + catch (Exception e) + { + requestState._exception = e; + requestState._response = null; + } + + // Signal waitable object, indicate operation completed and fire callback. + requestState._ldapAsync._manualResetEvent.Set(); + requestState._ldapAsync._completed = true; + + if (requestState._ldapAsync._callback != null && !requestState._abortCalled) + { + requestState._ldapAsync._callback(requestState._ldapAsync); + } + } return asyncResult; } @@ -404,31 +426,6 @@ public IAsyncResult BeginSendRequest(DirectoryRequest request, TimeSpan requestT throw ConstructException(error, operation); } - private void ResponseCallback(IAsyncResult asyncResult) - { - LdapRequestState requestState = (LdapRequestState)asyncResult.AsyncState; - - try - { - DirectoryResponse response = _fd.EndInvoke(asyncResult); - requestState._response = response; - } - catch (Exception e) - { - requestState._exception = e; - requestState._response = null; - } - - // Signal waitable object, indicate operation completed and fire callback. - requestState._ldapAsync._manualResetEvent.Set(); - requestState._ldapAsync._completed = true; - - if (requestState._ldapAsync._callback != null && !requestState._abortCalled) - { - requestState._ldapAsync._callback(requestState._ldapAsync); - } - } - public void Abort(IAsyncResult asyncResult) { if (_disposed) @@ -1404,7 +1401,7 @@ internal LdapMod[] BuildAttributes(CollectionBase directoryAttributes, ArrayList return attributes; } - internal DirectoryResponse ConstructResponse(int messageId, LdapOperation operation, ResultAll resultType, TimeSpan requestTimeOut, bool exceptionOnTimeOut) + internal async ValueTask ConstructResponseAsync(int messageId, LdapOperation operation, ResultAll resultType, TimeSpan requestTimeOut, bool exceptionOnTimeOut, bool sync) { var timeout = new LDAP_TIMEVAL() { @@ -1436,7 +1433,30 @@ internal DirectoryResponse ConstructResponse(int messageId, LdapOperation operat needAbandon = false; } - int error = LdapPal.GetResultFromAsyncOperation(_ldapHandle, messageId, (int)resultType, timeout, ref ldapResult); + int error; + if (sync) + { + error = LdapPal.GetResultFromAsyncOperation(_ldapHandle, messageId, (int)resultType, timeout, ref ldapResult); + } + else + { + // Given that we will be polling every millisecond, we need a stopwatch to make sure we track the timeout + timeout.tv_sec = 0; + timeout.tv_usec = 0; + error = 0; + Stopwatch watch = Stopwatch.StartNew(); + while (watch.Elapsed < requestTimeOut) + { + error = LdapPal.GetResultFromAsyncOperation(_ldapHandle, messageId, (int)resultType, timeout, ref ldapResult); + if (error == 0 || error == -1) + { + break; + } + await Task.Delay(1).ConfigureAwait(false); + } + watch.Stop(); + } + if (error != -1 && error != 0) { // parsing the result diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapPartialResultsProcessor.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapPartialResultsProcessor.cs index 3d6b85838ca013..b80b5995c38c5a 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapPartialResultsProcessor.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapPartialResultsProcessor.cs @@ -6,6 +6,7 @@ using System.Threading; using System.Collections; using System.Diagnostics; +using System.Threading.Tasks; namespace System.DirectoryServices.Protocols { @@ -135,7 +136,9 @@ private void GetResultsHelper(LdapPartialAsyncResult asyncResult) try { - SearchResponse response = (SearchResponse)connection.ConstructResponse(asyncResult._messageID, LdapOperation.LdapSearch, resultType, asyncResult._requestTimeout, false); + ValueTask vt = connection.ConstructResponseAsync(asyncResult._messageID, LdapOperation.LdapSearch, resultType, asyncResult._requestTimeout, false, sync: true); + Debug.Assert(vt.IsCompleted); + SearchResponse response = (SearchResponse)vt.GetAwaiter().GetResult(); // This should only happen in the polling thread case. if (response == null) diff --git a/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs b/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs index af065dfd6a393d..e59110192ad73d 100644 --- a/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs +++ b/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs @@ -2,9 +2,11 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Diagnostics; using System.DirectoryServices.Tests; using System.Globalization; using System.Net; +using System.Threading; using Xunit; namespace System.DirectoryServices.Protocols.Tests @@ -17,6 +19,12 @@ public partial class DirectoryServicesProtocolsTests [ConditionalFact(nameof(IsLdapConfigurationExist))] public void TestAddingOU() { + while(!Debugger.IsAttached) + { + Thread.Sleep(5000); + } + Debugger.Break(); + using (LdapConnection connection = GetConnection()) { string ouName = "ProtocolsGroup1"; @@ -534,7 +542,9 @@ private SearchResultEntry SearchOrganizationalUnit(LdapConnection connection, st { string filter = $"(&(objectClass=organizationalUnit)(ou={ouName}))"; SearchRequest searchRequest = new SearchRequest(rootDn, filter, SearchScope.OneLevel, null); - SearchResponse searchResponse = (SearchResponse) connection.SendRequest(searchRequest); + //SearchResponse searchResponse = (SearchResponse) connection.SendRequest(searchRequest); + var ar = connection.BeginSendRequest(searchRequest, PartialResultProcessing.NoPartialResultSupport, null, null); + SearchResponse searchResponse = (SearchResponse)connection.EndSendRequest(ar); if (searchResponse.Entries.Count > 0) return searchResponse.Entries[0]; From a439794f8264b6877806641bf372ab095dad2229 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 11 Jun 2020 16:20:20 -0700 Subject: [PATCH 2/5] Fix PR Feedback --- .../Protocols/ldap/LdapConnection.cs | 11 +++++++---- .../tests/DirectoryServicesProtocolsTests.cs | 11 ++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs index 5b4ae9aa8db735..d1855b154e038d 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs @@ -378,9 +378,9 @@ public IAsyncResult BeginSendRequest(DirectoryRequest request, TimeSpan requestT s_asyncResultTable.Add(asyncResult, messageID); - ResponseCallback(ConstructResponseAsync(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true, sync: false), requestState); + _ = ResponseCallback(ConstructResponseAsync(messageID, operation, ResultAll.LDAP_MSG_ALL, requestTimeout, true, sync: false), requestState); - static async void ResponseCallback(ValueTask vt, LdapRequestState requestState) + static async Task ResponseCallback(ValueTask vt, LdapRequestState requestState) { try { @@ -1440,10 +1440,12 @@ internal async ValueTask ConstructResponseAsync(int messageId } else { - // Given that we will be polling every millisecond, we need a stopwatch to make sure we track the timeout timeout.tv_sec = 0; timeout.tv_usec = 0; error = 0; + int iterationDelay = 1; + // Underlying native libraries don't support callback-based function, so we will instead use polling and + // use a Stopwatch to track the timeout manually. Stopwatch watch = Stopwatch.StartNew(); while (watch.Elapsed < requestTimeOut) { @@ -1452,7 +1454,8 @@ internal async ValueTask ConstructResponseAsync(int messageId { break; } - await Task.Delay(1).ConfigureAwait(false); + await Task.Delay(Math.Max(iterationDelay, 100)).ConfigureAwait(false); + iterationDelay *= 2; } watch.Stop(); } diff --git a/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs b/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs index e59110192ad73d..d5965e346f9eea 100644 --- a/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs +++ b/src/libraries/System.DirectoryServices.Protocols/tests/DirectoryServicesProtocolsTests.cs @@ -19,12 +19,6 @@ public partial class DirectoryServicesProtocolsTests [ConditionalFact(nameof(IsLdapConfigurationExist))] public void TestAddingOU() { - while(!Debugger.IsAttached) - { - Thread.Sleep(5000); - } - Debugger.Break(); - using (LdapConnection connection = GetConnection()) { string ouName = "ProtocolsGroup1"; @@ -542,9 +536,8 @@ private SearchResultEntry SearchOrganizationalUnit(LdapConnection connection, st { string filter = $"(&(objectClass=organizationalUnit)(ou={ouName}))"; SearchRequest searchRequest = new SearchRequest(rootDn, filter, SearchScope.OneLevel, null); - //SearchResponse searchResponse = (SearchResponse) connection.SendRequest(searchRequest); - var ar = connection.BeginSendRequest(searchRequest, PartialResultProcessing.NoPartialResultSupport, null, null); - SearchResponse searchResponse = (SearchResponse)connection.EndSendRequest(ar); + IAsyncResult asyncResult = connection.BeginSendRequest(searchRequest, PartialResultProcessing.NoPartialResultSupport, null, null); + SearchResponse searchResponse = (SearchResponse)connection.EndSendRequest(asyncResult); if (searchResponse.Entries.Count > 0) return searchResponse.Entries[0]; From ea942ce5d369066c341b959cdc3f026feca3f66e Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 11 Jun 2020 16:27:36 -0700 Subject: [PATCH 3/5] Fix case where timeout is set to infinite --- .../DirectoryServices/Protocols/ldap/LdapConnection.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs index d1855b154e038d..1049868b3c0a26 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs @@ -1442,15 +1442,16 @@ internal async ValueTask ConstructResponseAsync(int messageId { timeout.tv_sec = 0; timeout.tv_usec = 0; + // Initialize to 0 error = 0; int iterationDelay = 1; // Underlying native libraries don't support callback-based function, so we will instead use polling and // use a Stopwatch to track the timeout manually. Stopwatch watch = Stopwatch.StartNew(); - while (watch.Elapsed < requestTimeOut) + while (true) { error = LdapPal.GetResultFromAsyncOperation(_ldapHandle, messageId, (int)resultType, timeout, ref ldapResult); - if (error == 0 || error == -1) + if (error == 0 || error == -1 || (requestTimeOut != Threading.Timeout.InfiniteTimeSpan && watch.Elapsed < requestTimeOut)) { break; } From 7d161293aed8e1cd199ef9733f1661af7aa65c6f Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 11 Jun 2020 17:02:26 -0700 Subject: [PATCH 4/5] Fix condition and address more feedback --- .../Protocols/ldap/LdapConnection.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs index 1049868b3c0a26..9850e342d468eb 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs +++ b/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/ldap/LdapConnection.cs @@ -1442,8 +1442,6 @@ internal async ValueTask ConstructResponseAsync(int messageId { timeout.tv_sec = 0; timeout.tv_usec = 0; - // Initialize to 0 - error = 0; int iterationDelay = 1; // Underlying native libraries don't support callback-based function, so we will instead use polling and // use a Stopwatch to track the timeout manually. @@ -1451,12 +1449,15 @@ internal async ValueTask ConstructResponseAsync(int messageId while (true) { error = LdapPal.GetResultFromAsyncOperation(_ldapHandle, messageId, (int)resultType, timeout, ref ldapResult); - if (error == 0 || error == -1 || (requestTimeOut != Threading.Timeout.InfiniteTimeSpan && watch.Elapsed < requestTimeOut)) + if (error != 0 || (requestTimeOut != Threading.Timeout.InfiniteTimeSpan && watch.Elapsed > requestTimeOut)) { break; } - await Task.Delay(Math.Max(iterationDelay, 100)).ConfigureAwait(false); - iterationDelay *= 2; + await Task.Delay(Math.Min(iterationDelay, 100)).ConfigureAwait(false); + if (iterationDelay < 100) + { + iterationDelay *= 2; + } } watch.Stop(); } From 10cb66290fcbf58db675f809045e9cedaf838277 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 11 Jun 2020 20:35:17 -0700 Subject: [PATCH 5/5] Fixing netcoreap2.0 build error due to missing references --- .../src/System.DirectoryServices.Protocols.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj b/src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj index 03d310243f3b35..1fdfb6b7abc10d 100644 --- a/src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj +++ b/src/libraries/System.DirectoryServices.Protocols/src/System.DirectoryServices.Protocols.csproj @@ -100,6 +100,8 @@ + +