Close() should NEVER call Dispose()

[autoany]

I apologyze for not knowing how to properly adress this thing, please forward where appropriate.

Close() should NEVER call Dispose()
Dispose() or Dispose(bool) CAN call Close() but NEVER the other way around!

These (C/D) are two entirely different things, be it file or COM port or anything, especially when it is derived from Component.

runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs

Line 588 in 289aa17

Dispose();

There is bigger story about this, but the main logic is:
Close should not Dispose, because Dispose can (and often does) more things like GC.SuppressFinalize

runtime/src/libraries/System.ComponentModel.Primitives/src/System/ComponentModel/Component.cs

Line 74 in 289aa17

GC.SuppressFinalize(this);

How are you supposed to re-Open it? That should work, right?

---

....and ~Finalize() should never throw an exception, right?

Reality about SerialPort at least until .NET 4.5 when you unplug the USB - the whole App crashes and there was no defence.
At lest that got solved I hope:

runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs

Line 593 in 289aa17

if (disposing)

But wait, this gets called from Close as well, right?

runtime/src/libraries/System.ComponentModel.Primitives/src/System/ComponentModel/Component.cs

Line 86 in 289aa17

_site?.Container?.Remove(this);

runtime/src/libraries/System.ComponentModel.Primitives/src/System/ComponentModel/Component.cs

Line 89 in 289aa17

((EventHandler?)_events[s_eventDisposed])?.Invoke(this, EventArgs.Empty);

[dotnet-policy-service]

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

[huoyaoyuan]

Close() should NEVER call Dispose() Dispose() or Dispose(bool) CAN call Close() but NEVER the other way around!

These (C/D) are two entirely different things

They are usually alias, for example Stream.Dispose simply calls Close. Calling which from the other is just implementation detail.

Close should not Dispose, because Dispose can (and often does) more things like GC.SuppressFinalize

This is also implementation detail of the disposable pattern. Close, Dispose and finalizer are all doing the same thing. GC.SuppressFinalize is just an optimization to avoid duplicated check. It's also a standard part of the dispose pattern. You can learn more at https://learn.microsoft.com/dotnet/standard/design-guidelines/dispose-pattern and https://learn.microsoft.com/dotnet/standard/garbage-collection/implementing-dispose .

How are you supposed to re-Open it? That should work, right?

Re-opening is usually not a thing. Reusable objects must be specially designed other than non-reusable ones. For really-reusable objects like DbContext, Dispose will also reset its state for reuse.
Specially for GC.SuppressFinalize, there is GC.ReregisterForFinalize.

....and ~Finalize() should never throw an exception, right?

Ideally it should, but it's impractical to really get exception-free when operating with external resource. The common practice is to not let it run, by calling Dispose properly.

Reality about SerialPort at least until .NET 4.5 when you unplug the USB - the whole App crashes and there was no defence.

Any IO operation like network can get exception from physical connection lost, and not properly handling the exception will crash application. How is it related to Close or Dispose?

[autoany]

Re-opening is usually not a thing.

I have to disagree. SerialPort is Component you can put onto a Form, then assign a port, Open, Close, assign different port, Open, Close, ...reuse. Why not?

....and ~Finalize() should never throw an exception, right?

Ideally it should, but it's impractical to really get exception-free when operating with external resource. The common practice is to not let it run, by calling Dispose properly.

Reality about SerialPort at least until .NET 4.5 when you unplug the USB - the whole App crashes and there was no defence.

Any IO operation like network can get exception from physical connection lost, and not properly handling the exception will crash application. How is it related to Close or Dispose?

No Close or Dispose ever helped with SerialPort in .NET 2.0 upto .NET 4.0 (at least 3.5) when you unplugged the USB (with VCP), because it threw exception from finalizer in GC-thread, which crashed the app, no matter what you did.

EDIT:

there is GC.ReregisterForFinalize

See no such thing in Open

runtime/src/libraries/System.IO.Ports/src/System/IO/Ports/SerialPort.cs

Line 626 in 289aa17

public void Open()

[huoyaoyuan]

I have to disagree. SerialPort is Component you can put onto a Form

Component was a poor design in the history. Mixing every lifetime management with UI components is not a good practice.
In other words, you should explicitly open or close IO resources in your code, instead of make Form invoke it.

No Close or Dispose ever helped with SerialPort in .NET 2.0 upto .NET 4.0 (at least 3.5) when you unplugged the USB (with VCP), because it threw exception from finalizer in GC-thread, which crashed the app, no matter what you did.

The SerialPort object itself properly implements the dispose pattern. It gives up all the attempts when being finalized (disposing: false).
The problem comes from the internal SerialStream object. It attempts to flush its content without guard from exception. In current version, still only a finite set of exception codes are guarded:

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

Lines 724 to 734 in 289aa17

	// 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;
	}

Decompiling the implementation of .NET Framework 2.0, it only guards for ERROR_ACCESS_DENIED. The latest .NET Framework has fixed this: https://referencesource.microsoft.com/#System/sys/system/io/ports/SerialStream.cs,b120632fda7c01ba

In contrast, FileStream is implementing finalizer more properly. It ignores all failures when calling from finalizer:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 128 to 132 in 289aa17

	catch (Exception e) when (!disposing && FileStreamHelpers.IsIoRelatedException(e))
	{
	// On finalization, ignore failures from trying to flush the write buffer,
	// e.g. if this stream is wrapping a pipe and the pipe is now broken.
	}

there is GC.ReregisterForFinalize

See no such thing in Open

It's not fatal. The finalizer of SerialPort really does nothing.

All of the problems come from the implementation of SerialStream. It's not related to how Close or Dispose are called in SerialPort at all.

[autoany]

I have to disagree. SerialPort is Component you can put onto a Form

Component was a poor design in the history. Mixing every lifetime management with UI components is not a good practice. In other words, you should explicitly open or close IO resources in your code, instead of make Form invoke it.

My apologies, but that is not an argument, it is nonsense. (Make Close trully Close the port and not Dispose the object, problem solved. How can you blame anybody for thinking that Close is Close and not Dispose?)

But if you really want to convince me: Create simple Terminal app in WinForms using designer according to available documentation and suggestions. Document it and I will accept that I live in the past (.NET 2.0/3.5). Convince me, I beg you. Test it with CH340 and cheap replacements. I will accept the failure anyway, if you can proove it is "per design" and "working as expected", I will accept it.

---

No Close or Dispose ever helped with SerialPort in .NET 2.0 upto .NET 4.0 (at least 3.5) when you unplugged the USB (with VCP), because it threw exception from finalizer in GC-thread, which crashed the app, no matter what you did.

The SerialPort object itself properly implements the dispose pattern.

Bla, bla, bla. It (SerialPort or its internal SerialStream) threw exception in GC-thread. FACT.
Crashed the app and there was no defense, no code could solve that. FACT.

---

My solution: I repurposed and fixed your SerialStream to make it work. FACT.
I switched back to SerialPort in .NET 4.5, believeing everything is fine. FACT.
I switched back to my ComStream (my original work-around) again recently, because SerialPort in .NET 8/9 sucks the same way. FACT (EDIT: there is different problem, but it AGAIN sucks, mine still works)

---

Back to original: why should Close be Dispose?
(Do not be lazy, please. my logic: remove Close if it is Dispose!!!)

---

EDIT: there is a reason for this to even exist (not my code)
https://github.com/jcurl/RJCP.DLL.SerialPortStream/blob/master/code/SerialPortStream.cs

[huoyaoyuan]

My apologies, but that is not an argument, it is nonsense. (Make Close trully Close the port and not Dispose the object, problem solved. How can you blame anybody for thinking that Close is Close and not Dispose?)

Back to original: why should Close be Dispose?

It's the convention to do closing in Dispose. It's just an historical artifact that the actual closing work is inside the Dispose method rather than Close.
The invoking direction is also a historical artifact that can't be changed. Someone inheriting SerialPort and overriding Dispose is already depending on it.
Despite the design is non-ideal or broken, changing it will break other ones. Instead, it will be left as-is for its current state to make its behavior predictable, even being predictably "broken".

EDIT: there is a reason for this to even exist (not my code)
https://github.com/jcurl/RJCP.DLL.SerialPortStream/blob/master/code/SerialPortStream.cs

There are cases when types provided by the framework are unsuggested in favor of third party component, for example SmtpClient. The framework provided type has to be compatible to users who has adapted its quirks.

I do not have a time machine to answer or change things from the past.

[steveharter]

Changing existing semantics around Close-related methods and Dispose would be a large breaking change and would likely not be approved. The examples given are for code that is 20+ years old.

If there is a specific issue regarding certain types, such finalizers with SerialPort, then please log specific issue(s) for those.

[autoany]

It's the convention to do closing in Dispose. It's just an historical artifact that the actual closing work is inside the Dispose method rather than Close.

Bad convention and I think we actually agree on this.

Someone inheriting SerialPort and overriding Dispose is already depending on it.

Any proof anybody does that?
Why not creating new class that is better and make SerialPort obsolete?

Despite the design is non-ideal or broken, changing it will break other ones. Instead, it will be left as-is for its current state to make its behavior predictable, even being predictably "broken".

Changing existing semantics around Close-related methods and Dispose would be a large breaking change and would likely not be approved.

I almost expected that answer, did try anyway, execuse me for wasting your time, please.

If there is a specific issue regarding certain types, such finalizers with SerialPort, then please log specific issue(s) for those.

There currently is a problem in SerialPort with CH340 and there are known issues and planned changes... will see about that.

The problem comes from the internal SerialStream object. It attempts to flush its content without guard from exception. In current version, still only a finite set of exception codes are guarded:

That is most probably the reason why .NET 2.0/3.5 version crashed when you unplugged USB.

NOTE: I always try { inner.Dispose(); } catch in my Dispose(false) (and I think you now understand why).

Anyway, I accept the close (of this issue). Have a nice day :)