Connection not properly closed in right timed deactivate call.

[daniel-naegele]

If StompClient#deactivate() is called after the StompHandler#_connectToStomp() has been issued but the CONNECTED frame has not yet been received, the client is not sending the DISCONNECT command to the server. This is due to the fact, that the _connected variable is still false when StompHandler#dispose() is called.

Additionally, with SockJS this had the effect of the connection still remaining open (heartbeats where received from the server), even after StompHandler#_cleanUp was called. I don't know how or why this happens. But I don't know if this is reproducible for others.

Reproduce: Use this as your main method in example/main.dart:

void main() async {
  stompClient.activate();
  // Run next event loop
  Future.microtask(() => stompClient.deactivate());

  await Future.delayed(const Duration(seconds: 5));
  stompClient.activate();
}

[KammererTob]

Thanks for the report. I will check it out soon.

[KammererTob]

@daniel-naegele May i ask what server you are using? Normally it is acceptable for the client to disconnect by closing the socket connection. The DISCONNECT frame is only considered a graceful shutdown, but is not required. https://stomp.github.io/stomp-specification-1.2.html#DISCONNECT. Thus i feel like the current behavior should be sufficient according to the spec.

Maybe the issue with heartbeats are actually something different? Can you maybe elaborate what i should be seeing when using your reproduction?

[daniel-naegele]

Modified main.dart to reproduce, see log at the end of ticket:

void main() async {
  stompClient.activate();

  // Run next event loop, will execute before all other futures
  Future.microtask(() => stompClient.deactivate());
  // OR - execute in normal next event loop
  //   Future.delayed(const Duration(seconds: 0)).then((_) => stompClient.deactivate());
}

The problem is, that the deactivate call comes through before the _channel field of StompHandler has been assigned. Thus, _channel?.sink.close(); StompHandler#L311 get's called with the _channel still being null. Then, by the time the client connects and the still active check get's called, snippet following:

          if (!_isActive) {
            config.onDebugMessage(
                '[STOMP] Client connected while being deactivated. Will disconnect.');
            _handler?.dispose();
            return;
          }

The _handler then is already null, because of the previous deactivate call, see StompClient#L30, _handler = null;

We are using Spring Boot with the respective websocket, sockjs and stomp implementation.

Log of the program execution:

The Dart DevTools debugger and profiler is available at: http://127.0.0.1:35093/snvsVZvRdJQ=/devtools?uri=ws://127.0.0.1:35093/snvsVZvRdJQ=/ws
>>> ["CONNECT\naccept-version:1.0,1.1,1.2\nheart-beat:5000,5000\n\n\u0000"]
<<< o
<<< a["CONNECTED\nversion:1.2\nheart-beat:0,0\n\n\u0000"]
[STOMP] Client connected while being deactivated. Will disconnect.
<<< h
<<< h
...

[KammererTob]

@daniel-naegele Thanks for the detailed description. I understood what the issue was and was able to reproduce it. I have commited a proposed fix. Maybe you can have a quick look if this looks good to you as well. It also contains a unit test.

https://github.com/blackhorse-one/stomp_dart_client/commit/95c5292b5cde329ef4db5544381543243034a7d8

[daniel-naegele]

As far as I can tell, this looks good to me, thank you for the quick response and bug fix.

Will there be a new release on pub.dev soon?

Also, maybe a bit of an off topic question, but is there any difference in choosing normal websockets vs SockJS connections with this library, because the SockJS implementation also only uses websockets. Yes, maybe the overhead with SockJS is a bit larger but this should be neglectable. I think this should maybe be clarified in the readme, because at first glance I thought, that the SockJS implementation had most of the SockJS features, such as long polling, etc.

[KammererTob]

@daniel-naegele Yes. I will release 1.0.0 later this week.

Regarding SockJS: You are right. This library basically only implements/supports two aspects of the SockJS spec:

1. https://sockjs.github.io/sockjs-protocol/sockjs-protocol-0.3.3.html#section-36
2. https://sockjs.github.io/sockjs-protocol/sockjs-protocol-0.3.3.html#section-42

Internally we use https://pub.dev/packages/web_socket_channel for our WebSocket connection, which does not support SockJs in terms of a fallback for a real WebSocket connection. It basically only tells our library that we a) should expect/generate a different URL and b) need a slightly different parser for the messages we receive. I will add this fact to the Readme as well.

Thank you for your input.

[daniel-naegele]

Thanks again for this thorough response. To be honest, I think that for now the SockJS implementation is more than sufficient enough, as for most apps, websockets shouldn't be a problem.

Closing this issue.

[KammererTob]

@daniel-naegele 1.0.0 is released now.