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.

lib/RTCSession.js

lib/Notifier.js

orgads · 2025-12-25T11:00:04Z

@ikq thanks for the patience. I'll try 🤞 to finish the review and accept it in the following weeks. Can I ask you to not include any change in dist/ folder nor in package.json and to sync the branch with master?

Done

orgads · 2025-12-25T11:02:17Z

Hi @jmillan. Thanks for the review.

I work with @ikq at Audiocodes, and he agreed to let me take over this change as he has no time to address all the comments.

I pushed a new rebased revision, hopefully addressing all your comments. Please review.

jmillan · 2025-12-25T11:16:21Z

Thanks, I'll take some time on a further, and hopefully last review.

lib/Notifier.js

src/Subscriber.js

lib/Subscriber.js

jmillan · 2026-01-07T10:33:59Z

Currently UA fires newSubscribe with the incoming SUBSCRIBE request.

If SUBSCRIBE request does not have a Contact header, the request WONT be responded, hence breaking the SIP protocol. Contact header is verified in Notifier constructor which simply throws if it's missing. This kind of sanity checks should be done in Notifier.init_incoming(subscriber) which should reject the request or emit newSubscribe. This method can be a static method in Notifier.

Check a similar case in RtcSession: https://github.com/versatica/JsSIP/blob/master/lib/RTCSession.js#L3499

jmillan · 2026-01-07T11:52:23Z

@orgads, there may be some duplicated suggestion on my review due to a problem between GH and my side, sorry for that. Please simply resolve those duplicated.

orgads · 2026-01-07T16:38:07Z

I think all the comments should be now covered.

orgads · 2026-01-07T16:38:38Z

(I can't Resolve comments since I'm not the owner 🙄)

jmillan · 2026-01-07T17:03:56Z

(I can't Resolve comments since I'm not the owner 🙄)

I will, no worries.

src/Notifier.js

lib/Notifier.js

jmillan · 2026-01-08T09:08:03Z

src/Notifier.js

+      logger.debug(`emit "terminated" code=${C.SUBSCRIPTION_EXPIRED}, send final notify=true`);
+      this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);


Why not call this._terminateDialog(C.SUBSCRIPTION_EXPIRED) instead?

It passes true for sendFinalNotify, and also doesn't call this._dialog.terminate().

lib/Notifier.js

lib/Subscriber.js

src/Subscriber.js

lib/Subscriber.js

jmillan · 2026-01-09T14:26:10Z

Hi @orgads, I'm doing some modernisation on the library and I've moved code to src/ rather than lib/. Please adapt the PR at your convenience.

I don't plan to do any other change affecting this PR.

orgads · 2026-01-11T15:59:27Z

Hi @orgads, I'm doing some modernisation on the library and I've moved code to src/ rather than lib/. Please adapt the PR at your convenience.

I don't plan to do any other change affecting this PR.

Rebased.

src/Notifier.js

jmillan · 2026-01-12T09:06:13Z

src/Notifier.js

+      }
+
+      logger.debug(`emit "terminated" code=${C.SUBSCRIPTION_EXPIRED}, send final notify=true`);
+      this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);


I see no type definitions (.d.ts) for events in Notifier.d.ts. Can you please add them?

I wonder why the last argument is needed.

When a subscription expires, final NOTIFY should be sent.

The purpose of this flag is to allow the client to send final notify with custom body on subscription expiry and call terminate (without JsSip destroying the dialog before that).

An alternative suggestion I discussed with @ikq is to add a callback named fillFinalNotify or so that will be called inside terminateDialog, allowing to fill custom body.

What do you prefer (or just send an empty NOTIFY, not allowing customization)?

Then rather than doing:

this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);

And letting the user call this.terminate() I would:

// Emit expired rather than terminate. // The user can call `terminate()` within `expired` event callback. this.emit('expired'); // Call `terminateDialog()`. It won't have any effect if the user already called `terminate()`. this._terminateDialog(C.SUBSCRIPTION_EXPIRED);

NOTE: _terminateDialog() should check this._terminated and return if set.

Regarding _terminateDialog, not exactly. terminate must set _state to TERMINATED, because it is sent to the subscriber in sendNotify. But it does check for this._dialog, which should be enough.

Also, instead of calling _terminateDialog, I call terminate, to ensure we send a final notify.

The rest is done.

src/Subscriber.js

jmillan · 2026-01-12T09:23:35Z

src/Subscriber.js

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


Can you please point me where in the standard is this behaviour defined? The fact that SUBSCRIBE requests should be queued until the dialog is created?

The only use casi I find is that we want to terminate the subscription before receiving the response to the initial SUBSCRIBE, but than can be achieved on accepted and other event handlers.

This is done in order to support the case of multiple incoming SUBSCRIBE requests that are received before the other party responds. _dialog is initialized on response, and then we have to send an initial NOTIFY that matches the body of the subscribes.

So we need to store body and headers anyway. The queue is only needed to support multiple requests.

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

Sorry, I misunderstood the issue. If I understand correctly, it is for repeated calls to subscribe before the other party replies with 200 OK. So the first call sends SUBSCRIBE, then the next call should send an in-dialog SUBSCRIBE, but there is no dialog yet. So we need to wait for 200 OK, and then we send all the pending SUBSCRIBEs.

src/Subscriber.d.ts

src/Notifier.js

jmillan · 2026-01-12T10:41:17Z

src/Notifier.js

+      if (!this._dialog)
+      {
+        return;
+      }
+
+      logger.debug(`emit "terminated" code=${C.SUBSCRIPTION_EXPIRED}, send final notify=true`);
+      this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);


I definitely think this should call this._terminateDialog( C.SUBSCRIPTION_EXPIRED). I see no reason not to do it. Am I missing something?

See #711 (comment)

src/Subscriber.js

orgads

4 comments still pending. I'll consult with @ikq.

src/Notifier.js

src/Subscriber.d.ts

src/Notifier.js

src/Subscriber.js

jmillan · 2026-01-12T15:23:46Z

src/Notifier.js

+    return C;
+  }
+
+  get C()


This one after constructor please:

first static methods.

second constructor.

Then all other methods.

public firsts.

private later.

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?

jmillan · 2026-01-12T15:50:33Z

src/Notifier.js

+      }
+
+      logger.debug(`emit "terminated" code=${C.SUBSCRIPTION_EXPIRED}, send final notify=true`);
+      this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);


Then rather than doing:

this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);

And letting the user call this.terminate() I would:

// Emit expired rather than terminate. // The user can call `terminate()` within `expired` event callback. this.emit('expired'); // Call `terminateDialog()`. It won't have any effect if the user already called `terminate()`. this._terminateDialog(C.SUBSCRIPTION_EXPIRED);

NOTE: _terminateDialog() should check this._terminated and return if set.

jmillan · 2026-01-12T15:55:39Z

src/Subscriber.js

+
+    if (parsed === -1)
+    {
+      throw new TypeError('eventName - wrong format');


Suggested change

throw new TypeError('eventName - wrong format');

throw new TypeError('Missing Event header field');

jmillan · 2026-01-12T15:56:38Z

src/Subscriber.js

+    }
+  }
+
+  _terminateDialog(terminationCode, reason = undefined, retryAfter = undefined)


Can you please move this after terminate() definition?

jmillan · 2026-01-12T15:57:27Z

src/Notifier.js

+
+  static init_incoming(request, callback)
+  {
+    if (request.method !== JsSIP_C.SUBSCRIBE || !request.hasHeader('contact'))


Event header field should also be verified here.

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

jmillan reviewed Dec 19, 2025

View reviewed changes

lib/RTCSession.js Show resolved Hide resolved

lib/RTCSession.js Show resolved Hide resolved

lib/RTCSession.js Show resolved Hide resolved

jmillan reviewed Dec 19, 2025

View reviewed changes

lib/Notifier.js Outdated Show resolved Hide resolved

orgads force-pushed the subscribe_support branch from b2ef69c to d390a7c Compare December 25, 2025 10:55

jmillan reviewed Jan 7, 2026

View reviewed changes

orgads force-pushed the subscribe_support branch 2 times, most recently from fb75081 to 9c09c0f Compare January 7, 2026 16:36

jmillan reviewed Jan 8, 2026

View reviewed changes

orgads force-pushed the subscribe_support branch from 9c09c0f to ddfc457 Compare January 11, 2026 15:59

jmillan reviewed Jan 12, 2026

View reviewed changes

orgads force-pushed the subscribe_support branch from ddfc457 to c20b660 Compare January 12, 2026 13:52

orgads reviewed Jan 12, 2026

View reviewed changes

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

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

		logger.debug(`emit "terminated" code=${C.SUBSCRIPTION_EXPIRED}, send final notify=true`);
		this.emit('terminated', C.SUBSCRIPTION_EXPIRED, true);

	throw new TypeError('eventName - wrong format');
	throw new TypeError('Missing Event header field');

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 7, 2026 •

edited

Loading