Subscribe support #711

ikq · 2021-05-20T15:21:07Z

I added Subscriber.js Notifier.js to JsSIP

I tried write them according JsSIP style and lint errors.
Please take a look and let me know what needs to be fixed.

I already see that *.d.ts files are missing.

The work still in progress: I want add to SUBSCRIBE Contact +sip.instance
as you do in REGISTER

jmillan · 2021-05-21T05:57:27Z

JsSIP code must have the same style in order to make it readable and maintainable. Comments before making a deeper review:

Debug line as the first method call in al public method implementations.
New line after return.
New line before and after call to super().
First conditional in a block starts in new line and last ends in a new line.
Conditional blocks always contain curly brackets.
Comments in the line above when possible.
Calls to debug have usually a new line above and a new line below.

Please take a certain file (ie: RTCSession.js) as a reference and try to stick to its style.

jmillan · 2021-05-25T06:25:11Z

@ikq

Thanks for the cleanup, 👍 We'll review the PR as we can.

jmillan · 2021-05-27T05:55:46Z

@ikq,

Cosmetic comment, we use camelCase in API method arguments (not in 100% of the cases, I know, but we will get there). Could you please make such change?

jmillan

I've made the first review.

Please go through all the comments and modify as needed. We'll go for sencond review afterwards.

lib/Notifier.js

src/Notifier.js

lib/Notifier.js

lib/Subscriber.js

ikq · 2021-06-05T11:56:25Z

I mostly fixed the issues.

[I thought about dialog:
if you write in C++/java style, then you could add an abstract class dialog, and invite-dialog, subscriber and notifier dialogs would be his descendants.
The abstract dialog class could be something like stripped version of Dialog.js without actual method implementations]

About dialog, seems:

dialog interface:

id (string)
state (number)
receiveRequest receive incoming request with the id

other functionality:

check incoming requests
keep set of parameters to create outgoing request.

It's already implemented in Subscriber/Notifier.
[Not exactly as in Dialog.js, but the same functionality]

jmillan · 2021-06-08T12:58:22Z

Hi,

Super busy lately. Will come back to this when we have some time.

jmillan · 2021-06-22T14:11:27Z

H @ikq,

Dialog should be agnostic to the method. Why can't current Dialog implementation be used here? It has basically the sendRequest() method that you can call, and it will call the receiveRequest() method on your Subscriber/Notifier implementation.

ikq · 2021-06-22T15:03:25Z

Hmm...
I checked the Dialog.js code again.
sendRequest and receiveRequest can be used.

I just did not consider this possibility before because I found INVITE/CANCEL/ACK specific code and properties in the Dialog (e.g. subscriber used different dialog states set).

jmillan · 2021-06-22T16:21:53Z

There is INVITE related code in Dialog because a dialog for INVITE method needs to behave in certain way, different to generic dialogs, but that's an implementation detail. Other clases should be able to create dialogs the same way RTCSession does.

ikq · 2021-06-24T18:41:59Z

Hi Jose,
Now subscriber and notifier used Dialog.js
It passed my basic testing now.

I'll continue to test next week:

I cannot check subscriber authentication (my sip server don't request it).
subscriber with some production server.
subscriber with notifier.

ikq · 2021-07-07T15:09:43Z

Testing of the current version has been completed successfully.
(subscribe authentication was not checked)

ikq · 2021-07-10T13:17:00Z

subscribe authentication checked using special test.
IMHO: the subscriber/notifier is ready.

ikq · 2021-08-08T10:34:11Z

I see I should change debug/debugerror to logger...

jmillan · 2021-08-18T17:30:44Z

Hi @ikq,

We are super busy lately. Please keep using your branch and update this PR whenever you find any issue/enhancement.

Also, tests would accelerate the adoption.

ikq · 2021-08-20T07:37:14Z

I already use this code for my work and so far no problem.

Two browsers test can be taken from:
https://github.com/ikq/subscribe_notify_test

jmillan · 2021-08-23T09:38:19Z

@ikq, more than an external test I mean tests in JsSIP as part of this PR, testing the clases created here.

N4COM · 2021-09-22T09:22:52Z

Hi, is there some news? I'd like to test this functionality.

ikq · 2021-09-22T10:32:04Z

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

markusatm · 2021-09-22T10:49:13Z

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

N4COM · 2021-09-22T10:57:47Z

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

I'd like to use the official repository, so I'd like if it will be merged ASAP

ikq · 2021-09-22T11:28:26Z

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Yes, I'm not using the Configuration.contact_uri

In fact, I was in a hurry to do this work, and did not pay attention at all to that there is such a parameter in the configuration !

Let check, how the configuration used for INVITE:

UA.js:
   this._contact = {
      pub_gruu  : null,
      temp_gruu : null,
      uri       : this._configuration.contact_uri,

RTCSession.js:
  this._contact = this._ua.contact.toString({
      anonymous,
      outbound : true
    });

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

mitre-mgarber · 2021-09-22T12:15:18Z

@ikq if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

ikq · 2021-09-22T12:29:09Z

if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

Thank you, I will gladly accept your help.

I have provided an external test for 2 browsers: one is sending an SUBSCRIBE and the other is receiving it and sending a NOTIFY.

Seems in unit test should be simulated 2 instances of JsSIP one send/other receive.
[I don't know how it can be written.
Probably instead two JsSIP instances, can be used single that send the message itself ?]
;-)

stefang42 · 2021-09-22T13:04:27Z

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq Is there a specific reason to use Configuration.contact_uri? Why not just do it as in RTCSession, with this._ua.contact.toString? Then it would also use the gruu if one was provided. Or am I missing something?

ikq · 2021-09-23T16:39:24Z

Thank you markusatm for founding the problem.
Thank you stefang42, I use your fix to build Contact in subscriber and notifier.

src/Notifier.js

jmillan · 2021-09-24T10:53:40Z

Hi,

Yes, in order for this PR to be merged we need tests in JsSIP project.

You don't need two JsSIP instances, just one instance and you should inject a transport that you control (a FAKE transport) that you use to send JsSIP UA SUBSCRIBE|NOTIFY messages and receive and check what comes from JsSIP.

If you need some help please let us know.

ikq · 2021-09-25T20:52:23Z

Hi,
I added nodeunit test to test subscriber notifier communication.

Implemented loopSocket that send message itself, but modified Call-ID in each leg.
(without the modification method findDialog confuses SUBSCRIBE dialog with NOTIFY dialog)
subscriber and notifier run in the same JsSIP instance and send/receive SIP messages.

The test SIP sequence is:

SUBSCRIBE
SUBSCRIBE OK
NOTIFY
NOTIFY OK
unSUBSCRIBE (with expires 0)
unSUBSCRIBE OK
final NOTIFY (with Subscription-State: terminated)
final NOTIFY OK.

The unit test work silently, but if uncomment line JsSIP.debug.enable('JsSIP:*');
can be seen complete SIP communication.

SUBSCRIBE and NOTIFY headers checking:
url, method, contact, subscribe accept, notify subscription-state, content-type and body.
Checked order of event sequence.

src/Notifier.js

jmillan · 2026-01-12T15:30:48Z

src/Subscriber.js

+    {
+      logger.debug('enqueue subscribe');
+
+      this._queue.push({ body, headers: Utils.cloneArray(headers) });


Nop, incoming SUBSCRIBE request will end up creating a Notifier, not a Subscriber. Subscriber is only created locally. Right?

src/Notifier.js

src/Subscriber.js

src/Notifier.js

jmillan · 2026-01-13T10:20:01Z

The force-push-es are breaking the conversations which point to code.

orgads · 2026-01-13T10:22:50Z

Sorry, do you prefer to stack up commits instead?

src/Notifier.js

src/RTCSession.js

src/Subscriber.d.ts

src/Subscriber.js

jmillan · 2026-01-13T10:44:34Z

Sorry, do you prefer to stack up commits instead?

I would prefer to see individual commits. This PR will be rebased before merging anyway.

* Remove needless timeout * Reorder _terminateDialog * Reorder events * Revert unrelated change in RTCSession

src/Subscriber.js

src/Notifier.js

src/Subscriber.js

jmillan · 2026-01-14T12:33:50Z

Also please check the test error given by CI.

jmillan · 2026-01-14T15:24:29Z

NOTE: This is taking time @ikq @orgads, but the result is getting very nice. Thanks for your patience and dedication!

This reverts commit 061e3a2.

ikq · 2026-01-14T15:56:19Z

Thanks @jmillan and @orgads.

About SIP SUBSCRIBE, it can be used not only to open session and refresh before expiration,
but also can provide some information in SUBSCRIBE body. (in the same way as in NOTIFY).

You can send at random time SUBSCRIBE to provide to other side updated body.

src/Notifier.d.ts

jmillan

I think these are my last comments.

jmillan · 2026-01-16T11:55:38Z

src/Notifier.js

+    if (this._expires > 0)
+    {
+      // Set expires timer and time-stamp.
+      this._setExpiresTimer();
+    }


IMO this should be moved to start() after responding the SUBSCRIBE. Otherwise we may send the terminated NOTIFY before responding the SUBSCRIBE which is odd and probably not spec compliant.

jmillan · 2026-01-16T12:20:44Z

src/Notifier.js

+    let expires = Math.floor((this._expires_timestamp - new Date().getTime()) / 1000);
+
+    if (expires < 0)
+    {
+      expires = 0;
+    }


There could be a race condition here. Imagine that expires is 0 in line 277. The expires timer may run in the next loop iteration. We are going to:

Send this NOTIFY with expires 0.

In the next iteration, send another NOTFY with state terminate.

I know this is not likely to happen, but I would suggest having a private method such:

Suggested change

let expires = Math.floor((this._expires_timestamp - new Date().getTime()) / 1000);

if (expires < 0)

{

expires = 0;

}

let expires = Math.floor((this._expires_timestamp - new Date().getTime()) / 1000);

// expires_timer is about to trigger. Clean up the timer and terminate.

if (expires <= 0)

{

if (!this._expires_timer)

{

logger.error('expires timer is not set');

}

clearTimeout(this._expires_timer);

this.terminate(body, 'timeout');

}

else

{

this._sendNotify([ `;expires=${expires}` ], body);

}

Mmm, indeed this._expires_timer won't be set if Expires header field value in initial SUBSCRIBE is 0.

NOTE: This is handled in a previous comment.

jmillan · 2026-01-16T12:37:23Z

src/Notifier.js

+    // Create dialog for normal and fetch-subscribe.
+    this._dialog = new Dialog(this, subscribe, 'UAS');
+
+    if (this._expires > 0)


this._expires = parseInt(subscribe.getHeader('expires'));

Which means:

this._expires would be NaN if Expires header is not present since subscribe.getHeader() will return undefined. Check my proposal below.

this._expires would be negative if the value of Expires header is negative. We should handle that. Currently we are simply not setting the expires timer for negative value. IMO we should reject negatively if this happens, which can be done in checkSubscribe().

RFC6665 states that:

If no "Expires" header field is present in a SUBSCRIBE request, the implied default MUST be defined by the event package being used.

So constructor() should have a new NotifierOptions field defaultExpires which must be mandatory and used in case Expires header field is not set.

jmillan · 2026-01-16T12:40:55Z

src/Notifier.js

+    // Create dialog for normal and fetch-subscribe.
+    this._dialog = new Dialog(this, subscribe, 'UAS');


Being correct we should move dialog creation to the moment prior to sending the response in start().

jmillan reviewed Jun 1, 2021

View reviewed changes

ikq requested a review from jmillan August 3, 2021 15:49

mitre-mgarber mentioned this pull request Aug 19, 2021

Added RFC3842 - Message Waiting functionality #731

Closed

jmillan reviewed Sep 24, 2021

View reviewed changes

src/Notifier.js Show resolved Hide resolved

orgads force-pushed the subscribe_support branch 2 times, most recently from 273076d to 60a678e Compare January 12, 2026 15:44

jmillan reviewed Jan 12, 2026

View reviewed changes

src/Subscriber.js Outdated Show resolved Hide resolved

src/Subscriber.js Outdated Show resolved Hide resolved

src/Notifier.js Outdated Show resolved Hide resolved

orgads force-pushed the subscribe_support branch 2 times, most recently from c5160d4 to 6c1cfd7 Compare January 12, 2026 17:18

Subscribe support

faff4fe

orgads force-pushed the subscribe_support branch from 6c1cfd7 to faff4fe Compare January 12, 2026 17:19

jmillan reviewed Jan 13, 2026

View reviewed changes

src/Notifier.js Outdated Show resolved Hide resolved

src/RTCSession.js Outdated Show resolved Hide resolved

src/Subscriber.d.ts Show resolved Hide resolved

src/Subscriber.js Outdated Show resolved Hide resolved

orgads added 2 commits January 13, 2026 12:58

Subscribe: Review fixes

cbedc6e

* Remove needless timeout * Reorder _terminateDialog * Reorder events * Revert unrelated change in RTCSession

RtcSession: Add missing typedef import for IncomingRequest

061e3a2

jmillan reviewed Jan 13, 2026

View reviewed changes

src/Subscriber.js Outdated Show resolved Hide resolved

jmillan reviewed Jan 13, 2026

View reviewed changes

src/Notifier.js Show resolved Hide resolved

src/Notifier.js Outdated Show resolved Hide resolved

orgads added 2 commits January 13, 2026 20:57

Subscribe: Deduplicate checks in Notifier

9a608f1

Subscribe: Avoid terminating dialog before receiving final notify

5d80f75

jmillan reviewed Jan 14, 2026

View reviewed changes

orgads added 2 commits January 14, 2026 17:33

Revert "RtcSession: Add missing typedef import for IncomingRequest"

65a165d

This reverts commit 061e3a2.

Subscribe: Code cleanup

b4cbc1a

orgads force-pushed the subscribe_support branch from 0925d34 to b4cbc1a Compare January 14, 2026 15:39

jmillan reviewed Jan 14, 2026

View reviewed changes

src/Notifier.d.ts Outdated Show resolved Hide resolved

Notifier: Make state more coherent

4c3171c

jmillan reviewed Jan 16, 2026

View reviewed changes

-    let expires = Math.floor((this._expires_timestamp - new Date().getTime()) / 1000);
-    if (expires < 0)
-    {
-      expires = 0;
-    }
+    let expires = Math.floor((this._expires_timestamp - new Date().getTime()) / 1000);
+    // expires_timer is about to trigger. Clean up the timer and terminate.
+    if (expires <= 0)
+    {
+      if (!this._expires_timer)
+      {
+        logger.error('expires timer is not set');
+      }
+      clearTimeout(this._expires_timer);
+      this.terminate(body, 'timeout');
+    }
+    else
+    {
+       this._sendNotify([ `;expires=${expires}` ], body);
+    }

		// Create dialog for normal and fetch-subscribe.
		this._dialog = new Dialog(this, subscribe, 'UAS');

Subscribe support #711

Are you sure you want to change the base?

Subscribe support #711

Uh oh!

Conversation

ikq commented May 20, 2021

Uh oh!

jmillan commented May 21, 2021

Uh oh!

jmillan commented May 25, 2021

Uh oh!

jmillan commented May 27, 2021

Uh oh!

jmillan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikq commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Jun 8, 2021

Uh oh!

jmillan commented Jun 22, 2021

Uh oh!

ikq commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Jun 22, 2021

Uh oh!

ikq commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikq commented Aug 8, 2021

Uh oh!

jmillan commented Aug 18, 2021

Uh oh!

ikq commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmillan commented Aug 23, 2021

Uh oh!

N4COM commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusatm commented Sep 22, 2021

Uh oh!

N4COM commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitre-mgarber commented Sep 22, 2021

Uh oh!

ikq commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefang42 commented Sep 22, 2021

Uh oh!

ikq commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jmillan commented Sep 24, 2021

ikq commented Jun 5, 2021 •

edited

Loading

ikq commented Jun 22, 2021 •

edited

Loading

ikq commented Jun 24, 2021 •

edited

Loading

ikq commented Jul 7, 2021 •

edited

Loading

ikq commented Jul 10, 2021 •

edited

Loading

ikq commented Aug 20, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 22, 2021 •

edited

Loading

ikq commented Sep 23, 2021 •

edited

Loading

ikq commented Sep 25, 2021 •

edited

Loading

jmillan commented Jan 13, 2026 •

edited

Loading

jmillan commented Jan 14, 2026 •

edited

Loading

jmillan left a comment •

edited

Loading

jmillan Jan 16, 2026 •

edited

Loading

jmillan Jan 16, 2026 •

edited

Loading