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

UserAgent.stop() function takes too much time #863

Closed
slavikbialik opened this issue Oct 6, 2020 · 12 comments
Closed

UserAgent.stop() function takes too much time #863

slavikbialik opened this issue Oct 6, 2020 · 12 comments
Labels

Comments

@slavikbialik
Copy link
Contributor

Describe the bug
For some reason when using "UserAgent.stop()" function it takes around 30 seconds to stop everything until it responses with the appropriate promise.

What I'm trying to achieve is a full unregister command, where it not only make an unregister but also:

  1. Clearing all SIP subscriptions.
  2. Closing the web socket connection.

From what I understand, the "stop()" function does exactly that. Only it is taking too much time.

Logs
Here are the logs from the console: logs

Observed behavior
Only after around ~32 seconds we can see at: 15:14:05.797
The following log:
sip.subscribe-dialog | Timer N expired for SUBSCRIBE dialog. Timed out waiting for NOTIFY.
Only after the above log it is disposing the publishers and the transport (WS) level.
What makes me think that it may occur as a result of that SUBSCRIBE message that it's timer expired, but not entirely sure.

Environment Information

  • Kamailio 5.3.5 (Web Socket)
  • Google Chrome 85.0.4183.121
  • SIP.JS 0.17.1
@egreenmachine
Copy link
Collaborator

egreenmachine commented Oct 6, 2020

Confirmed it in the code...

@john-e-riordan this looks to be an order of operations issue.

At the top of user-agent.stop() we transition to the Stopped state.

We then cycle through registerers, sessions, then subscriptions.

The subscription.dispose() function rightfully sends a SUBSCRIBE with an Expires=0 header. That causes a NOTIFY to get sent to SIP.js indicating that subscription has closed. However there is a check in user-agent.onTransportMessage() that is checking if the user-agent is stopped. And then dropping the NOTIFY because of the user-agent state. Causing the subscriber to wait for the timeout.

We will work on getting this patched. Thanks.

@slavikbialik
Copy link
Contributor Author

cating that subscription has closed. However there is a check in user-agent.onTransportMessage() that is checking if the user-agent is stopped. And then dropping the`NOTIFY because of

Thank you very much for quickly responding and finding the root cause of the problem! Hope it will be fixed soon :)
By the way, I tested the stop() function when I'm not doing my initial SIP SUBSCRIBE that I'm also doing when making the REGISTER, and everything is working fast. But of course I'll need this SUBSCRIBE.
Thank again!

@egreenmachine
Copy link
Collaborator

To work around this you could dispose of your subscriptions manually before calling stop() on the user-agent.

@slavikbialik
Copy link
Contributor Author

To work around this you could dispose of your subscriptions manually before calling stop() on the user-agent.

Thanks! Already done, but not in an efficient way I think. If you can suggest me a better way it be very helpful.
I have a few subscriptions I'm making while the user is registered (like presence, conference and some other events).
Is there a way to itterate on all active subscriptions and call the dispose or unsubscribe method?
I saw that I can get the subscriptions list by doing: userAgentObject._subscriptions and it is getting some array of Subscribers and for some reason I cannot take it and do anything with it, like iterating or something else.

The way how I did it currently, but again, I don't like it, for each subscribe event type, I stored the subscriber into a different global variable in the class and for each subscriber variable I'm calling the unsubscribe method.

By the way, what's the difference between dispose and unsubscribe? I assume that dispose will not unsubscribe the subscription in the PBX. Right? If so, I think it's less recommended, because in my case, if I will not fully unsubscribe, the PBX will keep sending irrelevant NOTIFY messages to my Kamailio (which may lead to a total block because Kamailio will block my PBX if it'll overload him).

@SD-Gaming
Copy link

SD-Gaming commented Mar 19, 2021

To work around this you could dispose of your subscriptions manually before calling stop() on the user-agent.

Any progress😊? How to do this before the bug is fixed?

I don't have any subscription, but the UA.stop() is very slow too. Almost 1min...

Thank you.

@slavikbialik
Copy link
Contributor Author

To work around this you could dispose of your subscriptions manually before calling stop() on the user-agent.

Any progress😊? How to do this before the bug is fixed?

I don't have any subscription, but the UA.stop() is very slow too. Almost 1min...

Thank you.

Nope, this issues is not resolved yet.
But, you can do what I did... you can just destroy your objects manually one by one with the following logical order:

  1. subscriberObj.unsubscribe();
  2. registerer.unregister();
  3. userAgent.transport.disconnect();
  4. userAgent.stop();

Preferably, do every action with promises. I mean, use then so you can wait each action promise to get back and act accordingly to do the next step.

@kamer
Copy link

kamer commented Jun 17, 2021

I have the same issue. I don't have any subscriptions. For me, closing the WebSocket takes too long, (20-50 secs)

I tried:

  • destroying object manually as @slavikbialik suggests.
  • closing the WebSocket manually with userAgent.transport.ws.close() to make sure if my issue is related to WebSocket.
  • in Chrome and Firefox.

I don't have any specific configurations, just followed SIP.js guide to reproduce. I don't know if this helps or is related my issue but I tried to open a WebSocket with sip protocol without any library and closed it. They both lasted under 1 second.

wss

@john-e-riordan
Copy link
Collaborator

john-e-riordan commented Oct 18, 2022

For next release, planning on...

  1. To address the issue of the UserAgent stopping before the cleanup of subscriptions is done (and other dialogs for that matter), will move the state transition to the Stopped state to be the last step of stop() (it's currently the first step). This will require the user to make sure they are not calling start() again before stop() has resolved, but there is no way around that. However without adding an additional state into the mix (like Stopping) which might could allow us to throw an error, it's gonna be on the user to make sure they avoid that stop/start race and wait for stop() to resolve (which does not seem unreasonable). While adding a new state might be the ultimate solution, for now this is all that will be done.
  2. To address the issues of "it's taking too long for step X to complete and I don't want to wait", going to add a user agent option to skip the graceful shutdown and simply strand any dialogs or registrations (leaving the server side to presumably clean them up on its own) and otherwise not wait on async operations to complete (like web socket close). Default behavior will remain unchanged.

As mentioned, the user can cleanup everything on their own in a manner which works well for their application prior to calling stop() and option 2 can help avoid the built-in behavior when doing so.

Comments and/or feedback welcome.

@slavikbialik
Copy link
Contributor Author

@john-e-riordan thanks for your reply!
Regarding the 2nd thing you've mentioned, it means that if for example we have a registration, you'll add a flag that will skip the SIP un-registration process to the PBX and will dispose immediately the Registerer? (and of course the same with subscriptions and etc)
If so - this is great! Because one of the things we had to add in our SIP.js repo is a function we called it disposeWithoutUnregister that is doing the dispose of the object but skipping the unregister process because we had issues where the web socket become IDLE ("zombie") and the SIP REGISTER (expires: 0) is trying to be sent but of course cannot and it is making the client to be stuck on this process and it cannot continue.
Question is if it's possible from your end to do similar thing in the dispose functions for some relevant objects like subscribers, registerers, sessions and etc that if set to true (or something like that) it'll just clear the objects without doing any SIP actions.
In our product is is critical that the client will operate as fast as it can without anything that can make him stuck and so we could also finally can return using SIP.js package instead of having our own repository if possible. :)

Thanks again.

@john-e-riordan
Copy link
Collaborator

The flag simply skips disposal and otherwise does not wait on the socket to close when UserAgent.stop() is called. It's value us arguably dubious and it has no impact on anything else...

    // The default behavior is to cleanup dialogs and registrations. This is not that...
    if (!this.options.gracefulShutdown) {
      // Dispose of the transport (disconnecting)
      this.logger.log(`Dispose of transport`);
      this.transport.dispose().catch((error: Error) => {
        this.logger.error(error.message);
        throw error;
      });

      // Dispose of the user agent core (resetting)
      this.logger.log(`Dispose of core`);
      this.userAgentCore.dispose();

      // Reset dialogs and registrations
      this._publishers = {};
      this._registerers = {};
      this._sessions = {};
      this._subscriptions = {};

      this.transitionState(UserAgentState.Stopped);

      return Promise.resolve();
    }

I agree that attempting to send a request when it is known in advance that the transport is closed or otherwise cannot send the message should be avoidable, but gonna say that's a separate issue from the one I'm trying to address here.

john-e-riordan added a commit to john-e-riordan/SIP.js that referenced this issue Oct 21, 2022
- remove autoStart and autoStop
- address issues in onsip#863
@john-e-riordan
Copy link
Collaborator

Added to main branch and queued up to go out in next release.

@slavikbialik
Copy link
Contributor Author

@john-e-riordan Can we add also the use of gracefulShutdown inside the dispose functions of registerers, sessions, subscriptions and publishers so if we just call a dispose on Registerer for example it'll not do any SIP actions to do the unregistration process and will clear immediately the registerers array?

An additional small issue with the transport.dispose() when gracefulShutdown is false is that if the web socket is CONNECTED but not really working because the remote side went down, so the web socket in "zombie" state is that the dispose will take at least 30 seconds and SIP.js client will get stuck for that time period because behind the dispose it's still going to do the disconnect().
It's also very important for us, as we're using SIP.js for mission critical calls (like 911) and we have to be able to make fast actions (recovery) like when we're detecting transport disconnections or any other issues they may occur.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants