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

TcpListener should implement IDisposable #63114

Closed
geoffkizer opened this issue Dec 24, 2021 · 10 comments · Fixed by #88043
Closed

TcpListener should implement IDisposable #63114

geoffkizer opened this issue Dec 24, 2021 · 10 comments · Fixed by #88043
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@geoffkizer
Copy link
Contributor

geoffkizer commented Dec 24, 2021

EDITED 3/14/2022 by @stephentoub:

namespace System.Net.Sockets
{
    public class TcpListener
+        : IDisposable
    {
+         void IDisposable.Dispose() => Stop();
    }
}

TcpListener is essentially a convenience wrapper around a Socket in listen mode. Sockets are disposable, but TcpListener is not. Instead, you are expected to call the Stop() method.

Implementing IDisposable enables "using" syntax and matches common library practice.

Consider that the sample code for TcpListener explicitly uses a try/finally to ensure that Stop() is called properly: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.tcplistener?view=net-6.0

See also https://stackoverflow.com/questions/33667149/why-tcplistener-does-not-implement-idisposable

@geoffkizer geoffkizer added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Sockets labels Dec 24, 2021
@ghost
Copy link

ghost commented Dec 24, 2021

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

Issue Details

TcpListener is essentially a convenience wrapper around a Socket in listen mode. Sockets are disposable, but TcpListener is not. Instead, you are expected to call the Stop() method.

Implementing IDisposable enables "using" syntax and matches common library practice.

Consider that the sample code for TcpListener explicitly uses a try/finally to ensure that Stop() is called properly: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.tcplistener?view=net-6.0

See also https://stackoverflow.com/questions/33667149/why-tcplistener-does-not-implement-idisposable

Author: geoffkizer
Assignees: -
Labels:

api-suggestion, area-System.Net.Sockets

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 24, 2021
@geoffkizer
Copy link
Contributor Author

Note that if we implement this, we should also ensure that operations like Accept throw ObjectDisposedException if the TcpListener is disposed, for consistency with how IDisposable works in general and with Socket in particular. And we should disallow Start() after Dispose() -- today we allow you to call Start() again after Stop(), which isn't super useful.

In other words, Dispose() is not simply a synonym for Stop().

Note that today, if you call Accept after Stop(), you get an InvalidOperationException with the message "You must call the Start() method before calling this method." -- which is not particularly correct in describing the condition. So adding Dispose and using it instead of Stop() would actually result in a more useful exception here.

We could also consider deprecating Stop.

See also #63115.

@scalablecory
Copy link
Contributor

We could also consider deprecating Stop.

Take it further: we should consider deprecating the entirety of TcpClient and TcpListener. They don't have much value.

@geoffkizer
Copy link
Contributor Author

Take it further: we should consider deprecating the entirety of TcpClient and TcpListener. They don't have much value.

I have some thoughts on that, I'll file another issue on it.

I do think there's some value in having simple, high-level TCP APIs that allow you avoid dealing with Socket directly. But I think TcpClient and TcpListener don't do a great job of this -- TcpClient in particular.

@karelz
Copy link
Member

karelz commented Jan 6, 2022

Triage: TcpListener has very low value - it is very thin wrapper of Socket. Adding IDisposable makes sense to at least provide more value over direct Socket.
We might also consider obsoleting this class and replace by #63162.

@karelz karelz added this to the Future milestone Jan 6, 2022
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Jan 6, 2022
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 14, 2022
@bartonjs
Copy link
Member

Adding IDisposable seems good, but the consensus was to implicitly/publicly implement the interface, even though that means TcpListener will have two equivalent Stop members.

Because the type has already shipped, has existed for a long time, and has very few derived types in the wild, we're okay with saying it doesn't need to bother with the Basic Dispose Pattern (protected virtual void Dispose(bool)).

namespace System.Net.Sockets
{
    public class TcpListener
+        : IDisposable
    {
+         public void Dispose() => Stop();
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 11, 2023
@jannesrsa
Copy link

What is the expected behaviour if Dispose is called twice?

When implementing the Dispose pattern, there is: “A block for conditional return if object is already disposed.”

protected virtual void Dispose(bool disposing)
{
    if (_disposed)
    {
        return;
    }

    if (disposing)
    {
        // TODO: dispose managed state (managed objects).
    }

    // TODO: free unmanaged resources (unmanaged objects) and override a finalizer below.
    // TODO: set large fields to null.

    _disposed = true;
}

@stephentoub
Copy link
Member

Same as Stop; it ends up being a nop if you call it twice in a row.

@jannesrsa
Copy link

Does adding the IDisposable interface to an existing class constitute a breaking change? Are breaking changes handled differently?

The specific breaking change I'm referring to occurs when a project has AnalysisMode set to All and TreatWarningsAsErrors set to True. In this case, the build fails due to rule CA2000.

@stephentoub
Copy link
Member

Does adding the IDisposable interface to an existing class constitute a breaking change?

No

The specific breaking change I'm referring to occurs when a project has AnalysisMode set to All and TreatWarningsAsErrors set to True. In this case, the build fails due to rule CA2000.

Analyzers can flag all sorts of things, and the default severity for CA2000 is off. If someone chooses to enable it as an error and it flags something that breaks their build, that's on them.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 26, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 3, 2023
@karelz karelz modified the milestones: Future, 8.0.0 Jul 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants