Fix SerialPort memory leak when requests time out and no data is received

[bklop]

Fixes #55146

This also replaces the use of ConcurrentQueue with a synchronized Queue to ensure that completed requests (and related data, including any user-supplied buffers) are immediately eligible for GC. This is very important when using SerialPort on resource constrained IoT devices. Since the most common scenario is that of a single client thread calling Read and/or Write and waiting for the result to come back, the coarse locking on the queue should lead to any contention or performance problems in practice.

[ghost]

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

Issue Details

---

Fixes #55146

This also replaces the use of ConcurrentQueue with a synchronized Queue to ensure that completed requests (and related data, including any user-supplied buffers) are immediately eligible for GC. This is very important when using SerialPort on resource constrained IoT devices. Since the most common scenario is that of a single client thread calling Read and/or Write and waiting for the result to come back, the coarse locking on the queue should lead to any contention or performance problems in practice.

Author:	bklop
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[dnfadmin]

All CLA requirements met.

[kasperk81]

did you notice improvement in cpu usage too? (#2379)

[bklop]

did you notice improvement in cpu usage too? (#2379)

I don't think this patch will improve CPU usage in any way (even though I haven't tested it).

But I've also had problems with high CPU usage when calling SerialPort.ReadByte() in a loop. Instead I now read directly from the BaseStream and buffer the data read in my own code. This significantly reduced the CPU load.

[ghost]

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.

[danmoseley]

@bklop are you planning more work here or are you waiting on review?

[danmoseley]

@krwq can we merge, or do you plan to do manual tests or something?

[bklop]

@bklop are you planning more work here or are you waiting on review?

I was going to make some changes to the queue locking as suggested by @krwq but unfortunately life happened and other things got in the way :(. But I will have some time the coming week so I will try to make these changes ASAP.

[bklop]

I've now pushed my latest changes but I noticed that System.IO.Ports.Tests.SerialStream tests are not run as part of the CI build. I assume this is because of ConditionalFact(nameof(HasOneSerialPort))?

Unfortunately I don't have a serial port on my development machine so I can't run it locally either. Are these type of tests run somewhere else? Otherwise I can run some (informal) tests with a real serial port on a linux IoT device next week.

[bklop]

@danmoseley / @krwq I tested basic reading/writing with a real serial port on a linux IoT device, works fine. Is there anything more I need to do to get this merged? :)

[krwq]

LGTM we can merge once CI is green assuming you've manually done some basic testing. Unfortunately I don't have any setup handy to test this

[krwq]

note that if we want this change to be included in 7.0 release we have to finish testing by this Friday (Aug 12). Later this needs to go through separate approval process and be double merged in 7.0 branch and main (it's not horrible if we miss 7.0 since this is separate nuget package but will definitely be easier if we do)

[bklop]

It'd be great if this was in the 7.0 release (we currently have a workaround where we dispose and recreate the SerialPort instance to avoid memory leaks on a timeout, but we'd like to get rid of this). Can this be merged, or do you need to do more testing yourself?

[krwq]

@bklop thanks, try out the nightly package whenever available and validate this fixes your issue:
https://github.com/dotnet/runtime/blob/main/docs/project/dogfooding.md