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

Use ConcurrentDictionary in all platforms to make client edm model thread safe #995

Merged
merged 2 commits into from Nov 27, 2017

Conversation

Projects
None yet
3 participants
@tchuan
Contributor

tchuan commented Nov 15, 2017

Issues

This pull request fixes issue #990 .

Description

  • Generic Dictionary does not support multi thread read and write. .Net Standard 1.1 does support ConcurrentDictionary, which makes this a easy fix.

@biaol-odata biaol-odata requested a review from AlanWong-MS Nov 20, 2017

@biaol-odata

This comment has been minimized.

Member

biaol-odata commented Nov 20, 2017

@AlanWong-MS This is related to the issue #990 that you might have some context. Can you take a look? I am not sure whether the latest ODL completely has switched to .Net Standard 1.1.

@@ -21,13 +21,7 @@ namespace Microsoft.OData.Client
using Microsoft.OData.Edm;
using Microsoft.OData.Edm.Vocabularies;
using c = Microsoft.OData.Client;
#if PORTABLELIB

This comment has been minimized.

@AlanWong-MS

AlanWong-MS Nov 21, 2017

Contributor

@tchuan, just had a discussion with the team and we think the appropriate change is actually to make the preprocessor target specifically Windows Phone 8.0 instead of removing the preprocessor entirely. This is due to the fact that we still build a test project against Windows Phone 8.0 (using OData Client portable lib, non-Net Standard version) and even if we don't plan to ship the portable lib, we don't want to publish a broken test until we've removed the test project and related projects (which we currently don't have the resources to review at this time).

Therefore, I recommend changing the preprocessor directive from
#if PORTABLELIB
to
#if PORTABLELIB && WINDOWSPHONE

The WINDOWSPHONE constant is defined in the Windows Phone test project that I referred to.

@tchuan

This comment has been minimized.

Contributor

tchuan commented Nov 23, 2017

Updated based on @AlanWong-MS 's suggestion.

@AlanWong-MS AlanWong-MS merged commit ff9a1d4 into OData:master Nov 27, 2017

1 check passed

license/cla All CLA requirements met.
@AlanWong-MS

This comment has been minimized.

Contributor

AlanWong-MS commented Nov 27, 2017

Tests pass on our buddy build. Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment