bchavez / Coinbase.Pro

:chart_with_upwards_trend: A .NET/C# implementation of the Coinbase Pro API.
https://docs.pro.coinbase.com/
Other
67 stars 27 forks source link

Does the socket leak if I don't unsubscribe? #38

Closed granthoff1107 closed 3 years ago

granthoff1107 commented 3 years ago

I'm wondering if I need to unsubscribe my socket in addition to unsubscribing my events to prevent leaks. Take this code for instance:

    protected CoinbaseProWebSocket _socket;
    public TickerFrequencyBasedMetricBase(CoinbaseProWebSocket socket)
    {
        this._socket = socket;
    }

    public async virtual Task InitalizeSubscription()
    {
        var sub = this.GetSubscription();
        //send the subscription upstream
        this._socket.RawSocket.MessageReceived += RawSocket_MessageReceived;
        await this._socket.SubscribeAsync(sub);
    }

Now say I want to implement IDisposable:

    public void Dispose()
    {
            // To Avoid leaking we dispose of the events.
           this._socket.RawSocket.MessageReceived += RawSocket_MessageReceived;
    }

Do I Additionally need to unsubscribe to the subscrition as well:

        var sub = this.GetSubscription();
        this._socket.Unsubscribe(sub);

Also, how does the unsubscribe work, do I have to use an object with the same reference, or does it implement a quality comparer based on the internal json?

bchavez commented 3 years ago

You have an asymmetrical advantage in answering your question because you have a more complete picture of the code you posted than what I have access to. I think my help should only be really scoped to code that exists in this repository and not really include your code.

You should follow the same patterns as in the example here: https://github.com/bchavez/Coinbase.Pro/blob/master/Source/Examples/ResilientWebSocket.cs

As a general rule, whenever you subscribe an object to a C# event, you should duly -= unsubscribe the same object from the event to prevent memory leaks when you are done.

So, something obvious that pops out at me is looking at your code example here:

public void Dispose()
{
   // To Avoid leaking we dispose of the events.
   this._socket.RawSocket.MessageReceived += RawSocket_MessageReceived; // why +=???

Why does it make sense to += during disposal? The += operation does not prevent leaks, quite the opposite; in fact, exacerbates the situation. Again, I don't know how your code works and I don't have a complete picture of your code; so I don't know how to answer your question.

Also, I did not write the underlying .RawSocket WebSocket4Net; you'll have to check the code there and see if there's anything amiss.

All this to say is... I'm sorry, I don't know how to answer your question. You'll have to provide some kind of complete and concrete example that shows something wrong with the code in the Coinbase.Pro library.

granthoff1107 commented 3 years ago

Sorry, maybe you misunderstood me. The += was a typo.

My question is about code in the repo, What I want to know is whether or not I have to unsubscribe from the coinbase pro socket in the dispose method

this._socket.unsubscribe(sub)

Does that have any events it cleans up internally?

On Sat, Feb 20, 2021, 2:45 PM Brian Chavez notifications@github.com wrote:

You have an asymmetrical advantage in answering your question because you have a more complete picture of the code you posted than what I have access to. I think my help should only be really scoped to code that exists in this repository and not really include your code.

You should follow the same patterns as in the example here:

https://github.com/bchavez/Coinbase.Pro/blob/master/Source/Examples/ResilientWebSocket.cs

As a general rule, whenever you subscribe an object to a C# event, you should duly -= unsubscribe the same object from the event to prevent memory leaks.

So, something obvious that pops out at me is looking at your code example here:

public void Dispose() { // To Avoid leaking we dispose of the events. this._socket.RawSocket.MessageReceived += RawSocket_MessageReceived; // why +=???

Why does it make sense to += during disposal? That += does not certainly prevent leaks, quite the opposite; in fact, exacerbates the situation. Again, I don't know how your code works and I don't have a complete picture of your code; so I don't know how to answer your question.

Also, I did not write the underlying .RawSocket WebSocket4Net https://www.nuget.org/packages/WebSocket4Net/; you'll have to check the code there and see if there's anything amiss.

All this to say is... I'm sorry, I don't know how to answer your question. You'll have to provide some kind of complete and concrete example that shows something wrong with the code in the Coinbase.Pro library.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bchavez/Coinbase.Pro/issues/38#issuecomment-782739142, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFQZHOG7P3UWRIM3PJV64TTAAGN7ANCNFSM4X6BJQXQ .

bchavez commented 3 years ago

When you work with .RawSocket you are working with WebSocket4Net; not necessarily the Coinbase.Pro library.

Whether or not you subscribe or unsubscribe from .RawSocket.MessageReceived should not have any effect on the Coinbase.Pro library. In general, as best practice, when you are done with a subscription, you should unsubscribe.

The only time the Coinbase.Pro library cares about the underlying web socket is when .RawSocket is connecting; where we subscribe to .RawSocket.Opened and .RawSocket.Error events. Once the .RawSocket is connected; we immediately unsubscribe from .RawSocket.Opened and .RawSocket.Error events. When the .RawSocket is connected, the only thing Coinbase.Pro has is an object reference to .RawSocket WebSocket4Net which will be cleared out when CoinbaseProWebSocket.Dispose() is called.

Please read the code here: https://github.com/bchavez/Coinbase.Pro/blob/master/Source/Coinbase.Pro/WebSockets/CoinbaseProWebSocket.cs

My best advice is to follow the same patterns used in the ResilientWebSocket example with regards to disposal and re-use: https://github.com/bchavez/Coinbase.Pro/blob/master/Source/Examples/ResilientWebSocket.cs