System.IO.Ports: check device capabilities for DTR/DSR in SerialStream Constructor/dispose (Windows) by Benkol003 · Pull Request #122454 · dotnet/runtime

Benkol003 · 2025-12-11T14:00:18Z

See #121468
Fixes #27729

Benkol003 · 2025-12-11T14:02:18Z

@dotnet-policy-service agree

…n ctor/dispose (dotnet#121468)

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

Co-authored-by: kasperk81 <[email protected]>

krwq · 2026-01-14T09:29:45Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

@@ -634,17 +634,21 @@ internal SerialStream(string portName, int baudRate, Parity parity, int dataBits
                // set constant properties of the DCB
                InitializeDCB(baudRate, parity, dataBits, stopBits, discardNull);

-                DtrEnable = dtrEnable;
+                //if device doesnt support DTR and DTR is disabled, then dont try to set DTR
+                if (DtrEnable || ((_commProp.dwProvCapabilities & Interop.Kernel32.COMMPROP.PCF_DTRDSR) != 0))


I'm a bit confused on checking DtrEnable property in the if statement. Should this be local dtrEnable? I'm seeing you're assigning it two lines later but wouldn't this DtrEnable always evaluate to false given this is inside constructor and it's not assigned earlier?

Also I think it would be more appropriate to throw if we cannot do what user requested (since name suggests user wants it enabled it will be surprising if it's not in the end without explanation) rather than ignoring (we should match behavior with other properties and other OS implementations, even if it means less intuitive behavior).

I'd recommend doing something along the lines of (do it as early as appropriate):

if (dtrEnable && (_commProp.dwProvCapabilities & Interop.Kernel32.COMMPROP.PCF_DTRDSR) == 0)) { throw new WhateverApplicableException(); // probably invalid operation but be consistent }

I'm also not sure why RTS setting is related to DTR being enabled - this should probably also check for feature presence and throw if not available.

I'd recommend to also try adding test case but it currently requires either two serial ports cross connected (null model) or single serial port with RX and TX connected (loopback). I'd appreciate if you could run it as it's currently manual. I think tests might be a bit aggressive how they detect it so make sure to not run it on machine which has anything fragile connected through serial port (i.e. production machine). Note that given complex nature of tests here it's not strict requirement but it will be more than welcome.

For RTS I think you can use this flag:

PCF_RTSCTS 0x0002

krwq · 2026-01-14T09:31:56Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

-                            if (disposing)
-                                throw Win32Marshal.GetExceptionForLastWin32Error();
+                            // access denied can happen if USB is yanked out. If that happens, we
+                            // want to at least allow finalize to succeed and clean up everything


I'm missing the part where we do the cleanup referred in the comment

krwq · 2026-01-14T09:34:33Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

@@ -717,29 +721,33 @@ protected override void Dispose(bool disposing)

                    // turn off all events and signal WaitCommEvent
                    Interop.Kernel32.SetCommMask(_handle, 0);
-                    if (!Interop.Kernel32.EscapeCommFunction(_handle, Interop.Kernel32.CommFunctions.CLRDTR))
+                    //if device supports DTR then clear
+                    if ((_commProp.dwProvCapabilities & Interop.Kernel32.COMMPROP.PCF_DTRDSR) != 0)


consider adding explicit bool flag somewhere, i.e. _isXYZSupported

krwq

Nice work on finding good way to check for capability. I added some comments you might want to address

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot · 2026-01-14T10:04:51Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

+                    // query and cache the initial RtsEnable value
+                    // so that set_RtsEnable can do the (value != rtsEnable) optimization
+                    _rtsEnable = (GetDcbFlag(Interop.Kernel32.DCBFlags.FRTSCONTROL) == Interop.Kernel32.DCBRTSFlowControl.RTS_CONTROL_ENABLE);

-                // now set this.RtsEnable to the specified value.
-                // Handshake takes precedence, this will be a nop if
-                // handshake is either RequestToSend or RequestToSendXOnXOff
-                if ((handshake != Handshake.RequestToSend && handshake != Handshake.RequestToSendXOnXOff))
-                    RtsEnable = rtsEnable;
+                    // now set this.RtsEnable to the specified value.
+                    // Handshake takes precedence, this will be a nop if
+                    // handshake is either RequestToSend or RequestToSendXOnXOff
+                    if ((handshake != Handshake.RequestToSend && handshake != Handshake.RequestToSendXOnXOff))
+                        RtsEnable = rtsEnable;


The RTS handling code (lines 642-650) should not be inside the DTR capability check. RTS (Request To Send) and DTR (Data Terminal Ready) are independent control signals. A device might support RTS but not DTR, or vice versa. Placing RTS setup inside the DTR capability check means RTS won't be configured if the device doesn't support DTR, which is incorrect behavior.

I already also mentioned that in the comment above #122454 (comment)

Copilot · 2026-01-14T10:04:52Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

                    // turn off all events and signal WaitCommEvent
                    Interop.Kernel32.SetCommMask(_handle, 0);
-                    if (!Interop.Kernel32.EscapeCommFunction(_handle, Interop.Kernel32.CommFunctions.CLRDTR))
+                    //if device supports DTR then clear


The comment has a spelling error: "if" should be capitalized to "If" at the beginning of the sentence.

Suggested change

//if device supports DTR then clear

//If device supports DTR then clear

Copilot · 2026-01-14T10:04:52Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

+                //if device doesnt support DTR and DTR is disabled, then dont try to set DTR
+                if (DtrEnable || ((_commProp.dwProvCapabilities & Interop.Kernel32.COMMPROP.PCF_DTRDSR) != 0))
+                {
+                    DtrEnable = dtrEnable;

-                // query and cache the initial RtsEnable value
-                // so that set_RtsEnable can do the (value != rtsEnable) optimization
-                _rtsEnable = (GetDcbFlag(Interop.Kernel32.DCBFlags.FRTSCONTROL) == Interop.Kernel32.DCBRTSFlowControl.RTS_CONTROL_ENABLE);
+                    // query and cache the initial RtsEnable value
+                    // so that set_RtsEnable can do the (value != rtsEnable) optimization
+                    _rtsEnable = (GetDcbFlag(Interop.Kernel32.DCBFlags.FRTSCONTROL) == Interop.Kernel32.DCBRTSFlowControl.RTS_CONTROL_ENABLE);

-                // now set this.RtsEnable to the specified value.
-                // Handshake takes precedence, this will be a nop if
-                // handshake is either RequestToSend or RequestToSendXOnXOff
-                if ((handshake != Handshake.RequestToSend && handshake != Handshake.RequestToSendXOnXOff))
-                    RtsEnable = rtsEnable;
+                    // now set this.RtsEnable to the specified value.
+                    // Handshake takes precedence, this will be a nop if
+                    // handshake is either RequestToSend or RequestToSendXOnXOff
+                    if ((handshake != Handshake.RequestToSend && handshake != Handshake.RequestToSendXOnXOff))
+                        RtsEnable = rtsEnable;
+                }


This change introduces new behavior to check device capabilities for DTR/DSR support, but there are no corresponding tests to verify this logic works correctly. Tests should be added to cover scenarios such as: devices that don't support DTR with dtrEnable=false (should skip setting DTR), devices that support DTR with both dtrEnable values, and ensuring RTS is still set independently of DTR capability.

Copilot · 2026-01-14T10:04:52Z

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs

+                    //if device supports DTR then clear
+                    if ((_commProp.dwProvCapabilities & Interop.Kernel32.COMMPROP.PCF_DTRDSR) != 0)
                    {
-                        int hr = Marshal.GetLastPInvokeError();
-
-                        // access denied can happen if USB is yanked out. If that happens, we
-                        // want to at least allow finalize to succeed and clean up everything
-                        // we can. To achieve this, we need to avoid further attempts to access
-                        // the SerialPort.  A customer also reported seeing ERROR_BAD_COMMAND here.
-                        // Do not throw an exception on the finalizer thread - that's just rude,
-                        // since apps can't catch it and we may tear down the app.
-                        const int ERROR_DEVICE_REMOVED = 1617;
-                        if ((hr == Interop.Errors.ERROR_ACCESS_DENIED || hr == Interop.Errors.ERROR_BAD_COMMAND || hr == ERROR_DEVICE_REMOVED) && !disposing)
-                        {
-                            skipSPAccess = true;
-                        }
-                        else
+                        if (!Interop.Kernel32.EscapeCommFunction(_handle, Interop.Kernel32.CommFunctions.CLRDTR))
                        {
-                            // should not happen
-                            Debug.Fail($"Unexpected error code from EscapeCommFunction in SerialPort.Dispose(bool)  Error code: 0x{(uint)hr:x}");
+                            int hr = Marshal.GetLastPInvokeError();

-                            // Do not throw an exception from the finalizer here.
-                            if (disposing)
-                                throw Win32Marshal.GetExceptionForLastWin32Error();
+                            // access denied can happen if USB is yanked out. If that happens, we
+                            // want to at least allow finalize to succeed and clean up everything
+                            // we can. To achieve this, we need to avoid further attempts to access
+                            // the SerialPort.  A customer also reported seeing ERROR_BAD_COMMAND here.
+                            // Do not throw an exception on the finalizer thread - that's just rude,
+                            // since apps can't catch it and we may tear down the app.
+                            const int ERROR_DEVICE_REMOVED = 1617;
+                            if ((hr == Interop.Errors.ERROR_ACCESS_DENIED || hr == Interop.Errors.ERROR_BAD_COMMAND || hr == ERROR_DEVICE_REMOVED) && !disposing)
+                            {
+                                skipSPAccess = true;
+                            }
+                            else
+                            {
+                                // should not happen
+                                Debug.Fail($"Unexpected error code from EscapeCommFunction in SerialPort.Dispose(bool)  Error code: 0x{(uint)hr:x}");
+
+                                // Do not throw an exception from the finalizer here.
+                                if (disposing)
+                                    throw Win32Marshal.GetExceptionForLastWin32Error();
+                            }
                        }
                    }


This change introduces new behavior to check device capabilities for DTR/DSR support in Dispose, but there are no corresponding tests to verify this logic works correctly. Tests should be added to verify that devices without DTR support don't attempt to clear DTR during disposal, and that the existing error handling still works correctly.

krwq · 2026-01-15T09:21:38Z

@Benkol003 I think I already captured essence of copilot's feedback in my comments - feel free to ignore them. The tests is a hard subject because we don't have devices connected in CI - unless you find some smart way to emulate those but still tests wouldn't know which device have which capability so you'd essentially test feature with itself which makes it have less value - therefore manual test here is essential to bootstrap or to any changes related to feature presence checks

dotnet-policy-service · 2026-01-29T15:20:54Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

dotnet-policy-service · 2026-02-12T16:11:07Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 11, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 11, 2025

Benkol003 mentioned this pull request Dec 11, 2025

Catch exception in SerialStream constructor if DTR or RTS is not available (Windows) #121468

Closed

System.IO.Ports: check device capabilities for DTR/RTS before using i…

8229338

…n ctor/dispose (dotnet#121468)

Benkol003 force-pushed the System.IO.Ports/check-device-capabilities branch from b29505d to 8229338 Compare December 11, 2025 20:34

build-analysis bot mentioned this pull request Dec 11, 2025

"We stopped hearing from agent Azure Pipelines 32. Verify the agent machine is running and has a healthy network connection" dotnet/dnceng#1886

Open

3 tasks

Benkol003 changed the title ~~System.IO.Ports: check device capabilities for DTR/RTS in SerialStream Constructor/dispose (Windows)~~ System.IO.Ports: check device capabilities for DTR/DSR in SerialStream Constructor/dispose (Windows) Dec 17, 2025

Benkol003 changed the title ~~System.IO.Ports: check device capabilities for DTR/DSR in SerialStream Constructor/dispose (Windows)~~ System.IO.Ports: check device capabilities for DTR/RTS in SerialStream Constructor/dispose (Windows) Dec 17, 2025

vcsjones added area-System.IO.Ports and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 17, 2025

vcsjones reviewed Dec 17, 2025

View reviewed changes

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs Outdated Show resolved Hide resolved

Benkol003 changed the title ~~System.IO.Ports: check device capabilities for DTR/RTS in SerialStream Constructor/dispose (Windows)~~ System.IO.Ports: check device capabilities for DTR/DSR in SerialStream Constructor/dispose (Windows) Dec 18, 2025

fix formatting

52e3721

build-analysis bot mentioned this pull request Dec 18, 2025

Cannot find 'arm64-v8a' device dotnet/dnceng#2284

Open

3 tasks

kasperk81 reviewed Dec 20, 2025

View reviewed changes

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs Outdated Show resolved Hide resolved

kasperk81 reviewed Dec 20, 2025

View reviewed changes

src/libraries/System.IO.Ports/src/System/IO/Ports/SerialStream.Windows.cs Outdated Show resolved Hide resolved

Benkol003 and others added 2 commits December 27, 2025 22:24

fix formatting

0386801

Co-authored-by: kasperk81 <[email protected]>

fix formatting

07e6fa2

Co-authored-by: kasperk81 <[email protected]>

krwq reviewed Jan 14, 2026

View reviewed changes

krwq requested a review from Copilot January 14, 2026 09:53

Copilot started reviewing on behalf of krwq January 14, 2026 09:54 View session

krwq added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 14, 2026

krwq self-assigned this Jan 14, 2026

Copilot AI reviewed Jan 14, 2026

View reviewed changes

dotnet-policy-service bot added the no-recent-activity label Jan 29, 2026

dotnet-policy-service bot closed this Feb 12, 2026

	//if device supports DTR then clear
	//If device supports DTR then clear

Conversation

Benkol003 commented Dec 11, 2025

Uh oh!

Benkol003 commented Dec 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krwq Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

krwq Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

krwq Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

krwq left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

krwq Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

krwq commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Jan 29, 2026

Uh oh!

dotnet-policy-service bot commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krwq Jan 15, 2026 •

edited

Loading

krwq commented Jan 15, 2026 •

edited

Loading