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 | Change s_enclaveProviders to ConcurrentDictionary #1451

Merged
merged 1 commit into from
Jan 12, 2022
Merged

Fix | Change s_enclaveProviders to ConcurrentDictionary #1451

merged 1 commit into from
Jan 12, 2022

Conversation

johnnypham
Copy link
Contributor

Fixes #1444

@DavoudEshtehari
Copy link
Member

Could you include a test?

@johnnypham
Copy link
Contributor Author

Could you include a test?

Any suggestions on how to consistently force concurrent access to s_enclaveProviders?

@DavoudEshtehari
Copy link
Member

My suggestion here is to provide the race condition by sending parallel requests to s_enclaveProviders blended with delay and async call.

@johnnypham
Copy link
Contributor Author

My suggestion here is to provide the race condition by sending parallel requests to s_enclaveProviders blended with delay and async call.

Again, that doesn't consistently force concurrent access. I think the use case for a test should be reproducible on every run. I don't mind adding it but it wouldn't add any value imo. Essentially the test would just be verifying whether a ConcurrentDictionary is actually thread-safe.

@DavoudEshtehari
Copy link
Member

DavoudEshtehari commented Jan 5, 2022

In this case it could be a candidate for stress testing.
Just to be more precise, the extension methods' thread safety hasn't been guaranteed.

@DavoudEshtehari DavoudEshtehari added this to the 5.0.0-preview1 milestone Jan 10, 2022
@DavoudEshtehari DavoudEshtehari changed the title Change s_enclaveProviders to ConcurrentDictionary Fix | Change s_enclaveProviders to ConcurrentDictionary Jan 10, 2022
@JRahnama JRahnama modified the milestones: 5.0.0-preview1, 4.0.1 Jan 12, 2022
@DavoudEshtehari DavoudEshtehari merged commit 330de76 into dotnet:main Jan 12, 2022
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Jan 13, 2022
@DavoudEshtehari DavoudEshtehari modified the milestones: 4.0.1, 5.0.0-preview1 Jan 13, 2022
@johnnypham johnnypham deleted the threadsafeGetEnclaveProvider branch January 17, 2022 19:21
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnclaveDelegate.Crypto GetEnclaveProvider appears to not be thread safe
3 participants