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

WinHttpHandler: fix double -> uint conversions for ARM64 #45480

Merged
merged 5 commits into from
Dec 3, 2020

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Dec 2, 2020

According to the Explicit numeric conversions section of the C# specification, the unchecked case of a double -> uint cast goes as following:

  • If the value of the operand is NaN or infinite, the result of the conversion is an unspecified value of the destination type.
  • Otherwise, the source operand is rounded towards zero to the nearest integral value. If this integral value is within the range of the destination type then this value is the result of the conversion.
  • Otherwise, the result of the conversion is an unspecified value of the destination type.

Looks like we do not handle the Timeout.InfiniteTimeSpan.TotalMilliseconds == -1.0 case correctly for ReceiveDataTimeout and TcpKeepAlive* , the latter resulting in test failures on ARM64 machines.

Fixes #45249

Note:
The failing Libraries Test Run release coreclr windows arm64 Release job does not get executed on PR-s, only on refs/heads/master, which means there is no way for me to validate the change before merging it. (Unless someone can suggest a way to access an ARM64 windows machine.)

/cc @EgorBo @tannergooding if interested.

@ghost
Copy link

ghost commented Dec 2, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

According to the Explicit numeric conversions section of the C# specification, the unchecked case of a double -> uint cast goes as following:

  • If the value of the operand is NaN or infinite, the result of the conversion is an unspecified value of the destination type.
  • Otherwise, the source operand is rounded towards zero to the nearest integral value. If this integral value is within the range of the destination type then this value is the result of the conversion.
  • Otherwise, the result of the conversion is an unspecified value of the destination type.

Looks like we do not handle the Timeout.InfiniteTimeSpan.TotalMilliseconds == -1.0 case correctly for ReceiveDataTimeout and TcpKeepAlive* , the latter resulting in test failures on ARM64 machines.

Fixes #45249

Note:
The failing Libraries Test Run release coreclr windows arm64 Release job does not get executed on PR-s, only on refs/heads/master, which means there is no way for me to validate the change before merging it. (Unless someone can suggest a way to access an ARM64 windows machine.)

/cc @EgorBo @tannergooding if interested.

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

A tiny nit, otherwise LGTM

antonfirsov and others added 2 commits December 2, 2020 19:31
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@antonfirsov
Copy link
Member Author

Test failures are unrelated.

@safern
Copy link
Member

safern commented Dec 3, 2020

Failures are known and unrelated. Merging to unblock CI.

@safern safern merged commit 3574432 into dotnet:master Dec 3, 2020
@ViktorHofer
Copy link
Member

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 3, 2020

Test failures are unrelated.

@antonfirsov even if test failures are unrelated, in future please search for existing issues or create them and link to the affected build so that the failures are refcounted.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 2, 2021
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants