Skip to content
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

Fix | AE enclave retry logic not working for async queries #1988

Merged
merged 6 commits into from Apr 18, 2023

Conversation

David-Engel
Copy link
Contributor

This should address intermittent failures related to #1422

Summary:

The retry logic when a cached enclave session expired (8 hours) doesn't look like it was implemented properly for async queries. There were several problems.

  • The flow into the async retry path didn't work. The logic would simply bubble the RetryableEnclaveQueryExecutionException exception all the way up.
  • It didn't make sense for the async retry to follow a different flow than the sync retry logic (which can handle an async flow just fine).
  • The multiple-thread handling code in EnclaveProviderBase didn't account for async tasks that might be running on different threads. The purpose of the logic is to block simultaneous threads requesting duplicate enclave sessions when one session could be used across them all if other threads were made to wait a bit. But when an async retry came through, it would introduce a 15 second wait since the retry task was on a different thread.
  • During async retries, the async command count wasn't being incremented even though it had already been decremented during the original failure. Code comments indicated it was intentional at one point, but it isn't correct in the current flow.
  • The retry mechanism for expired session caches wasn't being tested for expiry between commands nor for expiry during a narrow window between when we have the metadata to create the session and when we detect that a parameter will require generating an enclave package. It was this small window that was resulting in the Enclave session is null during query execution. errors. If the session cache entry expired at just the right time, the retry logic path would not even be hit.

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Patch coverage: 80.68% and project coverage change: +0.13 🎉

Comparison is base (d9ab5a9) 70.46% compared to head (c0ea3f7) 70.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1988      +/-   ##
==========================================
+ Coverage   70.46%   70.60%   +0.13%     
==========================================
  Files         306      306              
  Lines       61563    61800     +237     
==========================================
+ Hits        43382    43634     +252     
+ Misses      18181    18166      -15     
Flag Coverage Δ
addons 92.88% <ø> (+0.49%) ⬆️
netcore 73.42% <82.69%> (+0.15%) ⬆️
netfx 69.19% <82.22%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 74.00% <77.77%> (+0.38%) ⬆️
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 83.68% <79.06%> (+0.22%) ⬆️
.../SqlClient/AzureAttestationBasedEnclaveProvider.cs 80.00% <100.00%> (+0.61%) ⬆️
...Microsoft/Data/SqlClient/EnclaveDelegate.Crypto.cs 90.52% <100.00%> (+3.42%) ⬆️
...rc/Microsoft/Data/SqlClient/EnclaveProviderBase.cs 92.45% <100.00%> (+3.99%) ⬆️
...t/Data/SqlClient/NoneAttestationEnclaveProvider.cs 95.34% <100.00%> (+4.65%) ⬆️
.../SqlClient/VirtualSecureModeEnclaveProviderBase.cs 85.11% <100.00%> (-0.60%) ⬇️

... and 48 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama JRahnama added this to the 5.2.0-preview1 milestone Apr 14, 2023
@Kaur-Parminder
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Kaur-Parminder Kaur-Parminder merged commit 938b166 into dotnet:main Apr 18, 2023
132 checks passed
@David-Engel David-Engel deleted the AERetries branch April 19, 2023 00:11
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
@DavoudEshtehari DavoudEshtehari added this to In progress in SqlClient v5.2 via automation Sep 29, 2023
@DavoudEshtehari DavoudEshtehari added the 🐛 Bug! Something isn't right ! label Sep 29, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 25, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 27, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 30, 2023
@DavoudEshtehari DavoudEshtehari moved this from In progress to Done in SqlClient v5.2 Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug! Something isn't right !
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants