stomp-js / stompjs

Javascript and Typescript Stomp client for Web browsers and node.js apps
Apache License 2.0
773 stars 82 forks source link

Client reconnecting #112

Closed kosaa closed 5 years ago

kosaa commented 5 years ago

https://github.com/stomp-js/stompjs/blob/f11ad889211b9a8bd5c90d7309a23b832e1b60af/src/client.ts#L414

Hello It is ok that reconnecting basics on setTimeout? Its called only once. In this place should be used setInterval?

kum-deepak commented 5 years ago

It is supposed to work like following:

Are you facing any issues?

kosaa commented 5 years ago

In my case there is only one attempt for trying connecting again. I test it with following steps:

  1. create client with reconnectDelay default (5s)
  2. turn off local server or wifi
  3. turn on local server after 10 seconds

version 1

return new Promise<CompatClient>((resolve, reject) => {
    const socket = new SockJS(url);
    const stompClient = Stomp.over(socket);
    stompClient.configure({
        reconnectDelay: 5000
    });
    stompClient.activate();
    stompClient.connect({}, () => {
        resolve(stompClient);
    });
});

version 2

return new Promise<CompatClient>((resolve, reject) => {
    const socket = new SockJS(url);
    const stompClient = Stomp.over(socket);
    stompClient.connect({}, () => {
        resolve(stompClient);
    });
});

I do something wrong?

kum-deepak commented 5 years ago

In general I can see one mistake in both the versions. When using SockJS you need to pass a factory that returns a SockJS instance. For an example using compat mode, please see https://stomp-js.github.io/api-docs/latest/classes/Stomp.html#over. This is needed for re-connection to work.

Version 1 has more issues - it mixes compatibility syntax with new syntax. When using compatibility syntax, do not mix new syntax. If you are writing a new application, switch to new syntax. There is a guide at https://stomp-js.github.io/guide/stompjs/2018/09/08/upgrading-stompjs.html. For SockJS with newer syntax also see https://stomp-js.github.io/guide/stompjs/rx-stomp/ng2-stompjs/2018/09/10/using-stomp-with-sockjs.html.

kum-deepak commented 5 years ago

There is a side note - unrelated to your issue. Promises allow them to be resolved only once. However, with re-connection, for each successful connection you would need a notification. I could not, therefore, find a suitable pattern with Promises.

So, I switched to rxjs. If you like that type of working please see https://github.com/stomp-js/rx-stomp.

kosaa commented 5 years ago

Everything works when I replaced:

const socket = new SockJS(url);
const stompClient = Stomp.over(socket);

with:

const stompClient = Stomp.over(() => new SockJS(url));

About promise, I want to have only one connection in whole application and all parts of code use this single promise. Its a good practise?

kum-deepak commented 5 years ago

Using the same client in whole application is a good idea.

With re-connection it may get tricky. For example when it reconnects, all queues would need to be subscribed again. Also, if it is in the process of getting reconnected, sending a message will fail with an exception.

rx-stomp tries to simplify some of these. If during your development you find that Promises are not enough, this is where you might find some of the answers.

kosaa commented 5 years ago

thanks for help @kum-deepak

@offtopic e.g. angular display warnings if you do somethings wrong, and is easy to detect

Maybe this could be useful in the project?

if (arg != function) {
    console.warn('Constructor argument should be a factory method. See %docu_link%');
}
kum-deepak commented 5 years ago

If you are using it with angular you should consider ng2-stompjs (https://stomp-js.github.io/guide/ng2-stompjs/2018/11/04/ng2-stomp-with-angular7.html)

kum-deepak commented 5 years ago

Good idea :+1: