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

dispose always triggers the DisconnectionHappened #114

Open
BasieP opened this issue Jun 28, 2022 · 3 comments
Open

dispose always triggers the DisconnectionHappened #114

BasieP opened this issue Jun 28, 2022 · 3 comments

Comments

@BasieP
Copy link

BasieP commented Jun 28, 2022

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.

@JacquelineCasey
Copy link
Contributor

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.

@JacquelineCasey
Copy link
Contributor

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.

JacquelineCasey added a commit to JacquelineCasey/websocket-client that referenced this issue Jul 4, 2022
- Updated a test to catch this issue
- Updated the WebsocketClient.cs accordingly
@JacquelineCasey
Copy link
Contributor

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.

Marfusios pushed a commit that referenced this issue Oct 13, 2022
- Updated a test to catch this issue
- Updated the WebsocketClient.cs accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants