Draft | Addressing name pipe failures on sync and async calls. #1880
Conversation
JRahnama
requested review from
lcheunglci,
DavoudEshtehari,
David-Engel and
roji
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
==========================================
- Coverage 70.69% 69.97% -0.73%
==========================================
Files 292 292
Lines 61732 61732
==========================================
- Hits 43643 43197 -446
- Misses 18089 18535 +446
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
while we are at this there is PipeSecurity class which was available in netcoreapp 1.1 and it came back again in net6 and net7. We set ACL in native code, but not in netcore, Discretionary Access Control List (DACL). Something that needs to be brought up. |
DavoudEshtehari
force-pushed
the
np-async
branch
2 times, most recently
from
6149613
to
b218d31
Compare
JRahnama
changed the title
This is fairly common and much better than attempting to use a heuristic approach. It can be annoying to write but as long as you're only working within the managed implementation and don't need to change the interface used by native it should just turn out to be code plumbing. |
|
@Wraith2 so you are pro passing a flag from inside OpenAsync and pass it all the way here? Actually, there is a flag, but mid-way its value is set to be true all the time. |
|
It seems like a reasonable approach. It is at least worth trying. |
|
Temporary disabled test ConcurrentExecution |
- Setup Parameterization so PipeOptions can be configured for Async or WriteThrough
Change to a more precise variable name
Add the setting of the Asynchronous Variable assignment to SqlInternalConnectionTds
|
@JRahnama I think the problem with the test is that it is simply calling async methods in the context of sync methods and we are starving the thread pool. I can repro this locally by increasing the concurrentExecution number. If you SetMinThreads to >= concurrentExecution, the test succeeds. Remember, the default system thread count is # of CPU cores and our test VMs only have 2 cores. Another option is probably to split the test into separate sync and async versions and convert the async one to fully async. But even that option isn't certain with our sync over async issues. |
While working on some intermittent failures on test pipelines on, concurrent test runs for retry logic, I noticed there is
PipeOptions.AsynchronousinSNINPHandleclass ctr. That causes the tests to fail at some random runs. There is an option to pass a variable to decide on sync/async calls, but that approach needs more investigation as the value of async passed down to other functions inTDSParser.Connectfor the functionc call_physicalStateObj.CreatePhysicalSNIHandleis set to be alwyas true.My main concern with this change: what would be the effect of this on async calls that happen after openAsync? will taking out the pipeoptions for asynchronous calls will cause any damage on the async pattern, tasks or threads?
on some other notes:
In the very same class
SNINPHandleon the Receive function when the packet length is 0, stream is closed by server, the driver gets the last Win32Exception and throws that as exception. This does not seem like a very good approach to me. There could be some other problem happening in between. For example,There is no process at the other end of the pipewhen a wrong password is used. How other drivers have solved this issue?To repro the issue make sure youa re using named pipes and managed SNI is in use. The test that fails intermittently is:
ConcurrentExecutation in SqlConnectionReliablityTest
cc: @roji, @Wraith2 , @David-Engel , @DavoudEshtehari