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

Revert commit 4b29352e. #17

Closed
wants to merge 1 commit into from

Conversation

dsandras
Copy link
Contributor

There is no reason why we should not create new
TCP Connections on notifications. Imagine an architecture with two
redundant servers handling eachother part of the calls. Dialog notifications
would only work when the call is going through the server which handled the
subscription.

If accepted, please credit Damien Sandras from Be IP s.a. @ http://www.beip.be

There is no reason why we should not create new
TCP Connections on notifications. Imagine an architecture with two
redundant servers handling eachother part of the calls. Dialog notifications
would only work when the call is going through the server which handled the
subscription.
@saghul
Copy link
Member

saghul commented Jul 16, 2013

There is one really good reason NOT to do this: TCP is blocking in OpenSIPS. Most of the time it's clients connected to servers, and it's really easy to completely hog OpenSIPS by creating X subscriptions and tearing down the TCP connections.

This wasn't done arbitrarily, and unless OpenSIPS is modified to have non-blocking TCP operations we cannot simply apply this.

@saghul saghul closed this Jul 16, 2013
@dsandras
Copy link
Contributor Author

Then you break the module behavior as soon as a TCP connection is broken, which is a workaround more than a fix.

If I was you, I would create a module option to determine the behavior to follow.

@saghul
Copy link
Member

saghul commented Jul 16, 2013

A module parameter was my first implementation but after discussing it with @bogdan-iancu I ended up doing it like that. I'm not opposed to adding a module parameter though :-)

@dsandras
Copy link
Contributor Author

Well, I was discussing this with Bogdan the other day, and he did not seem to be aware that you could not have notifications emitted by two different servers for the same presentity.

I guess he will have to decide on this.

Could you reopen the pull request ? One fix triggers a potential security problem, the other fix is a modifying the expected behavior of the module...

@dsandras
Copy link
Contributor Author

However, thinking about it, there will only be notifications through
NOTIFY if there are prior subscriptions with SUBSCRIBE. And nothing in
the standard RFCs indicate that the SUBSCRIBE connection should stay
active all the time. Not all SIP clients maintain the connection up, and
configuring OpenSIPS for keep-alive, connection reuse and persistent
connections is not straightforward or the default.

That also means that any time a TCP connection will break, for whatever
reason, the UA will keep the subscription in its table for one hour.
During that time, OpenSIPS will probably notify for events, and if the
connection is down, it won't recreate a new one. It will thus fail and
remove the active subscription from the watchers table, leaving the UA
in an incoherent state for one hour.

From my point of view, this is a bug...

Le 16/07/13 17:29, Saúl Ibarra Corretgé a écrit :

A module parameter was my first implementation but after discussing it
with @bogdan-iancu https://github.com/bogdan-iancu I ended up doing
it like that. I'm not opposed to adding a module parameter though :-)


Reply to this email directly or view it on GitHub
#17 (comment).

@saghul
Copy link
Member

saghul commented Jul 17, 2013

The standard doesn't say the connection has to be active all the time, I didn't say otherwise. This is a workaround to help mitigate OpenSIPS hanging because all processes are blocked on TCP operations.

Now, nobody who is behind NAT can receive an incoming TCP connection to handle that NOTIFY so running into this problem is very easy.

Feel free to send a patch adding a module parameter for this (preserving the current behavior as default) but I'll not be reverting this patch for the reasons explained above.

@46labs 46labs mentioned this pull request Oct 7, 2013
@mnunzi mnunzi mentioned this pull request Nov 7, 2013
@apsaras apsaras mentioned this pull request Mar 26, 2014
@ankogan ankogan mentioned this pull request Feb 6, 2016
@apsaras apsaras mentioned this pull request Mar 5, 2016
@caloveri caloveri mentioned this pull request Jul 24, 2020
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

Successfully merging this pull request may close these issues.

2 participants