Marfusios/websocket-client

dispose always triggers the DisconnectionHappened

BasieP opened this issue · 3 comments

Hi there,

When I do a StartOrFail() with a bad uri (i.e. 'wss://bla/'). It triggers a DisconnectionHappened (this is correct).

But then, when I exit the scope, my client gets disposed. And it triggers again.
This should not happen, because the connection was not there. The client was not connected. Therefor it cannot disconnect.

It should only trigger it when it is onnected and I dispose the object.

I'd like to add that I experienced a seemingly related issue.
I have IsReconnectionEnabled set to false.
When the server disconnects, DisconnectionHappened triggers, as it should. I then exit the scope, and get another DisconnectionHappened trigger, which seems incorrect.

Ok, I fixed it on my machine. However, I am having some trouble with git/github:

Websocket.Client.Tests % git status     
On branch bugfix/double_disconnect
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean
Websocket.Client.Tests % git push --set-upstream origin bugfix/double_disconnect
remote: Permission to Marfusios/websocket-client.git denied to jackcasey067.
fatal: unable to access 'https://github.com/Marfusios/websocket-client/': The requested URL returned error: 403

I can't really figure it out, so I'd love help. However, if someone just wants to take my changes, that would be fine too.

I updated a test in ConnectionTests.cs to catch this bad behavior (see bottom):

[Fact]
        public async Task Stopping_ShouldWorkCorrectly()
        {
            var disconnectionCount = 0;

            using (var client = _context.CreateClient())
            {
                client.ReconnectTimeout = TimeSpan.FromSeconds(7);

                string received = null;
                var receivedCount = 0;
                var receivedEvent = new ManualResetEvent(false);
                DisconnectionInfo disconnectionInfo = null;

                client.MessageReceived
                    .Where(x => x.MessageType == WebSocketMessageType.Text)
                    .Subscribe(msg =>
                    {
                        _output.WriteLine($"Received: '{msg}'");
                        receivedCount++;
                        received = msg.Text;
                    });

                client.DisconnectionHappened.Subscribe(x =>
                {
                    disconnectionCount++;
                    disconnectionInfo = x;
                });

                await client.Start();

                _ = Task.Run(async () =>
                {
                    await Task.Delay(200);
                    client.IsReconnectionEnabled = false;
                    await client.Stop(WebSocketCloseStatus.InternalServerError, "server error 500");
                    receivedEvent.Set();
                });

                receivedEvent.WaitOne(TimeSpan.FromSeconds(30));

                // check that reconnection is disabled
                await Task.Delay(8000);
                Assert.Equal(1, receivedCount);
                Assert.Equal(1, disconnectionCount);
                Assert.Equal(DisconnectionType.ByUser, disconnectionInfo.Type);
                Assert.Equal(WebSocketCloseStatus.InternalServerError, disconnectionInfo.CloseStatus);
                Assert.Equal("server error 500", disconnectionInfo.CloseStatusDescription);
                Assert.False(client.IsRunning);
                Assert.False(client.IsStarted);
            }

            // Disposing a disconnected socket should not cause DisconnectionHappened to trigger.
            await Task.Delay(200);
            Assert.Equal(1, disconnectionCount);
        }

And then I fixed it in a function in WebsocketClient.cs (see if statement):

public void Dispose()
        {
            _disposing = true;
            Logger.Debug(L("Disposing.."));
            try
            {
                _messagesTextToSendQueue?.Writer.Complete();
                _messagesBinaryToSendQueue?.Writer.Complete();
                _lastChanceTimer?.Dispose();
                _cancellation?.Cancel();
                _cancellationTotal?.Cancel();
                _client?.Abort();
                _client?.Dispose();
                _cancellation?.Dispose();
                _cancellationTotal?.Dispose();
                _messageReceivedSubject.OnCompleted();
                _reconnectionSubject.OnCompleted();
            }
            catch (Exception e)
            {
                Logger.Error(e, L($"Failed to dispose client, error: {e.Message}"));
            }

            if (IsRunning) 
            {
                _disconnectedSubject.OnNext(DisconnectionInfo.Create(DisconnectionType.Exit, _client, null));
            }

            IsRunning = false;
            IsStarted = false;
            _disconnectedSubject.OnCompleted();
        }

Hope this helps.

Ok I resolved my git woes. Instead of trying to make a branch on this repo, I forked it. PR #116 now has my proposed changes.