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

[7.0] Fix reporting of an async IO timeout error on Windows (SerialPort) #81745

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

kouvel
Copy link
Member

@kouvel kouvel commented Feb 7, 2023

  • Port of Fix reporting of an async IO timeout error on Windows (SerialPort) #81744
  • When an async SerialPort IO operation times out, it reports the timeout in the IO completion with an NTSTATUS value of WAIT_TIMEOUT (258)
  • In the thread pool when using GetQueuedCompletionStatusEx, the NTSTATUS value was being checked against STATUS_SUCCESS to determine success, so the WAIT_TIMEOUT was reported as an error. This leads to a different exception being thrown, compared to before when GetQueuedCompletionStatus was used.
  • Fixed to use similar logic to the SDK's NT_SUCCESS macro, which treats the WAIT_TIMEOUT value as a success, which is similar to what GetQueuedCompletionStatus does
  • There are already tests that verify this behavior in System.IO.Ports tests, though they are currently disabled due to instabilities. I have verified locally that the relevant failures are fixed and that there are no new failures in those tests.

Port of fix for #80079

Customer Impact

A SerialPort IO operation that times out raises an IOException instead of the documented TimeoutException. Some customers have reported the issue and that it has broken several apps using SerialPort on Windows. There is a runtime config option that can be configured to revert the change in behavior, which serves as a workaround. Another workaround is to catch the IOException and check the error code.

Regression?

Yes, from 6.0

Testing

Verified with the repro that the behavior is the same as before

Risk

Low. .NET 6 uses GetQueuedCompletionStatus, and after the change the behavior is closer to what GetQueuedCompletionStatus does.

…ort`)

- Port of dotnet#81744
- When an async `SerialPort` IO operation times out, it reports the timeout in the IO completion with an `NTSTATUS` value of `WAIT_TIMEOUT` (258)
- In the thread pool when using `GetQueuedCompletionStatusEx`, the `NTSTATUS` value was being checked against `STATUS_SUCCESS` to determine success, so the `WAIT_TIMEOUT` was reported as an error. This leads to a different exception being thrown, compared to before when `GetQueuedCompletionStatus` was used.
- Fixed to use similar logic to the SDK's `NT_SUCCESS` macro, which treats the `WAIT_TIMEOUT` value as a success, which is similar to what `GetQueuedCompletionStatus` does
- There are already tests that verify this behavior in `System.IO.Ports` tests, though [they are currently disabled](https://github.com/dotnet/runtime/blob/b39d6a6eb44860746e91e5ce4f585beff33d1f63/src/libraries/System.IO.Ports/tests/Support/TCSupport.cs#L108-L118) due to instabilities. I have verified locally that the relevant failures are fixed and that there are no new failures in those tests.

Relevant to dotnet#80079
@kouvel kouvel added this to the 7.0.x milestone Feb 7, 2023
@kouvel kouvel self-assigned this Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

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

Issue Details
  • Port of Fix reporting of an async IO timeout error on Windows (SerialPort) #81744
  • When an async SerialPort IO operation times out, it reports the timeout in the IO completion with an NTSTATUS value of WAIT_TIMEOUT (258)
  • In the thread pool when using GetQueuedCompletionStatusEx, the NTSTATUS value was being checked against STATUS_SUCCESS to determine success, so the WAIT_TIMEOUT was reported as an error. This leads to a different exception being thrown, compared to before when GetQueuedCompletionStatus was used.
  • Fixed to use similar logic to the SDK's NT_SUCCESS macro, which treats the WAIT_TIMEOUT value as a success, which is similar to what GetQueuedCompletionStatus does
  • There are already tests that verify this behavior in System.IO.Ports tests, though they are currently disabled due to instabilities. I have verified locally that the relevant failures are fixed and that there are no new failures in those tests.

Port of fix for #80079

Author: kouvel
Assignees: kouvel
Labels:

area-System.Threading

Milestone: 7.0.x

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Feb 7, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 7, 2023
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.4 Feb 7, 2023
@carlossanlop
Copy link
Member

Approved by Tactics for 7.0.4.
Signed-off by area owners.
Common and S.P.C. code modified, so no OOB changes needed.
CI failures unrelated: #81544 #81391 #81807 #81123
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 75979eb into dotnet:release/7.0 Feb 9, 2023
@kouvel kouvel deleted the NtStatusFix7 branch February 9, 2023 17:59
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Threading Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants