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

Enhance invariant mode checks #1917

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Feb 4, 2023

fixes #1913

Adds checks for the appcontext switch, the environment switch and encloses the original check in a try-catch block to make it compatible with net6 which changed how invariant mode culture creation works per the information in the original thread.

/cc @jeroen-mostert

@codecov
Copy link

codecov bot commented Feb 4, 2023

Codecov Report

Base: 69.62% // Head: 69.96% // Increases project coverage by +0.33% 🎉

Coverage data is based on head (4aec7c2) compared to base (5ff68eb).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1917      +/-   ##
==========================================
+ Coverage   69.62%   69.96%   +0.33%     
==========================================
  Files         292      292              
  Lines       61750    61759       +9     
==========================================
+ Hits        42995    43209     +214     
+ Misses      18755    18550     -205     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.33% <50.00%> (+0.60%) ⬆️
netfx 67.87% <ø> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs 78.74% <50.00%> (-0.38%) ⬇️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs 30.35% <0.00%> (-14.29%) ⬇️
...ta/SqlClient/SqlConnectionPoolGroupProviderInfo.cs 42.50% <0.00%> (-5.00%) ⬇️
...Microsoft/Data/ProviderBase/DbConnectionFactory.cs 17.04% <0.00%> (-3.41%) ⬇️
...ient/netfx/src/Microsoft/Data/SqlClient/SqlUtil.cs 57.14% <0.00%> (-0.25%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 89.41% <0.00%> (-0.16%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 67.97% <0.00%> (-0.09%) ⬇️
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs 87.36% <0.00%> (+0.52%) ⬆️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 44.19% <0.00%> (+20.89%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jeroen-mostert
Copy link

As per the discussion in runtime#81429, checking for the environment setting or app switch is not the right way of doing this -- the app may also be trimmed to not support globalization-invariant mode, and then the switches will have no effect, while this code would incorrectly detect that GIM is enabled. I think adding the exception handler is the only thing that needs to happen here. For some libraries having an unavoidable exception would be annoying, but since SqlClient is going to effectively rethrow it anyway that's not an issue.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 5, 2023

If a user specifies either the environment variable or the appcontext switch then they are expecting the app to run in invariant mode. If for some reason like trimming it doesn't run in invariant mode the intent was still that it should do, so we should still throw the error. Accidentally working because the app ignores the user can be as confusing or worse than not working.

I also don't know if trimming and aot are supported scenarios for this library. There is reflection usage in the library and it isn't annotated for trimming at all.

@jeroen-mostert
Copy link

jeroen-mostert commented Feb 5, 2023

If for some reason like trimming it doesn't run in invariant mode the intent was still that it should do, so we should still throw the error. Accidentally working because the app ignores the user can be as confusing or worse than not working.

Whether or not the app will run in GIM based on trimming and/or switches and what the intent was is between the app and the user (and the deployer), though. It's not SqlClient's responsibility to point out they're doing something interesting by refusing to work when it could. If this is to be considered some kind of problem that should be addressed, either the app itself should check or the runtime should compile (untrimmable) code in to that effect. I can easily imagine scenarios where, for example, the environment variable is set as a default, but trimming has forced never-invariance on a particular app so it works everywhere. Is that the most elegant way of doing things? No, but why should SqlClient then refuse to play ball? The "imagine if every library did this" argument applies here.

I also don't know if trimming and aot are supported scenarios for this library. There is reflection usage in the library and it isn't annotated for trimming at all.

That's another matter; even if it doesn't work today there's no real reason for this code to be a deliberate roadblock. If checking for the switch and the environment variable is not a reliable way to check for GIM, then this code shouldn't do it, because checking whether GIM is in effect is the only thing it needs (arguably even less than that, but building in full support for a limited set of cultures is a lot more involved; that's something for a future release if it's ever built at all).

Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Wraith2!

Copy link
Contributor

@lcheunglci lcheunglci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Wraith2

@JRahnama JRahnama merged commit a4f18ca into dotnet:main Feb 11, 2023
@David-Engel David-Engel added this to the 5.2.0-preview1 milestone Mar 20, 2023
@JackInTheBoxBen
Copy link

I have been getting this error after building a Linux images and pushing it to Kubernetes. I'm not sure what I am supposed to do in this case? Am I supposed to use the invariant environment variable or not? Also is this a case issue en-us is not en-US. Can some one please clarify what the corrective action is? Or will this all be fixed and go away in the next release? Is that out yet? Any help is appreciated as I am stuck here without a solution in sight. Thank you.

@Wraith2 Wraith2 deleted the fix-invariantdetection branch March 29, 2023 16:47
@Wraith2
Copy link
Contributor Author

Wraith2 commented Mar 29, 2023

You need to install the icu libraries on your container. sql server connections require the ability to create comparers which can deal with the data encoded in the database, you can't run in invariant mode. this PR only changes the checks so they report the error more cleanly on newer runtimes.

@JackInTheBoxBen
Copy link

JackInTheBoxBen commented Mar 29, 2023 via email

@JackInTheBoxBen
Copy link

JackInTheBoxBen commented Mar 29, 2023 via email

@JackInTheBoxBen
Copy link

JackInTheBoxBen commented Mar 29, 2023 via email

@JackInTheBoxBen
Copy link

JackInTheBoxBen commented Mar 29, 2023 via email

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.

Detecting globalization-invariant mode fails
6 participants