feat(session): add support for multiple sessions per client #332

princjef · 2017-09-07T00:54:46Z

Addresses #331 by adding the ability to create multiple sessions on the same client/connection and providing policy options for reestablishing the session on end. This feature set is designed to better align the capabilities of amqp10 with the session pattern supported by Azure Service Bus and bring the retry logic inline with what is used for reattach and reconnect.

Features

Add createSession() in AMQPClient to create user sessions
- Support custom policy overrides
Allow an optional session to be passed in on createSender(), createSenderStream(), createReceiver(), createReceiverStream()
Create the default session associated with the client only when needed
restart policy option for reestablishing the session after it is ended

Tests

createSession()
Optional session parameter for createSender(), createSenderStream(), createReceiver(), createReceiverStream()
Creating default client session on demand
Session policy - restart

Please take a look and let me know what you think of the change. I'm open to changing the design/approach if people have other thoughts.

This change is

princjef · 2017-09-07T17:18:27Z

I also pulled in the change from #330 for now so that I can get useful information from the Travis CI build. That change looks good to me and I think it's worth merging in before this goes in.

princjef · 2017-09-08T21:17:49Z

Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

lib/policies/policy.js, line 127 at r1 (raw file):

      enableSessionFlowControl: true,
      closeOnLinkError: false,
      reattach: null

I'm not wild about calling this reattach, since the concept of attach/detach applies to links, not sessions. The equivalent for the session terminology would be rebegin or remap, but neither really feels right to me. Thoughts?

Comments from Reviewable

princjef · 2017-09-27T00:26:54Z

I decided to remove the endOnLinkDetach policy from the session because it's not a part of the spec and was causing more problems than it was solving. The other changes have stood up well to my testing though

pierreca · 2018-01-23T01:44:09Z

lib/amqp_client.js

+            resolve();
+          });
+
+          self._session.begin(self._session.policy || self.policy.session);


self._session.policy || self.policy.session [](start = 30, length = 43)

if these 2 are the same, no problem. if the existing session doesn't have a policy object, apply the configured one. i get that part - but what if self._session.policy is set to something that is incompatible with self.policy.session (which i assume is the desirable policy) ? should these objects be merged instead ? or why keep the existing policy object at all?

alright the tricky part here is that the policy isn't applied at creation time, but when begin() is called. therefore, we need to provide the policy each time we start the session again (or it gets cleared out).

for the session created with the client, the policy is never specified directly by the user so i'm fine with doing a merge if we want. the user-created sessions are trickier because they can be specified independently and should supersede that of the client. in those cases we really only want to apply the client's policy if we don't have one (which should never happen but i've been caught by those assumptions too many times to not put in the check).

sound reasonable?

ha I see. the philosophy of policies as i understand it though is that they are merged and what is passed around are policy overrides - so starting with the client policy and then merging/overriding with the session policy would feel like the natural thing to do. but i defer to your judgement.

In reply to: 163131314 [](ancestors = 163131314)

i think it's reasonable to do the merge on the built-in one (this line). the user one is weird because the begin() here is a hidden operation. the policy for the session is specified separately so i don't want to override it if it's already there. i'll add a comment to that one

pierreca · 2018-01-23T01:49:37Z

lib/amqp_client.js

+
+      if (promises.length > 0) {
+        Promise.all(promises).then(function() {
+          self.emit('connected');


self.emit('connected'); [](start = 10, length = 23)

unrelated but puzzling nonetheless to me: why would the "connected" state of the client be attached to sessions rather than connection or links?

yeah this one is an artifact of how the Promise-based design works. it's trying to make sure things are in a usable state when the promise returns. since the session is baked into the connection by the library, that means both the connection and the session have to be ready. i considered making it lazy, but didn't know that i'd be able to get away with it without some subtle breaking changes

actually i did make it lazy. the problem is reconnect because the session is already there

pierreca · 2018-01-23T03:01:28Z

test/unit/test_session.js

@@ -22,7 +22,7 @@ var test = {
    connect: { options: { containerId: 'test' } },
    senderLink: { attach: { name: 'sender' } },
    receiverLink: { attach: { name: 'receiver' } }
-  }, DefaultPolicy)
+  }, DefaultPolicy),


, [](start = 19, length = 1)

typo?

probably had something else and took it out. good catch

pierreca · 2018-01-23T03:08:08Z

reestablish? the spec talks about establishing and ending sessions

In reply to: 328216141 [](ancestors = 328216141)

pierreca

princjef · 2018-01-23T17:54:46Z

ah i like that. reestablish sounds better to me than restart

Comments from Reviewable

princjef · 2018-01-23T18:00:20Z

lib/amqp_client.js, line 176 at r2 (raw file):

Previously, princjef (Jeff Principe) wrote…

i think it's reasonable to do the merge on the built-in one (this line). the user one is weird because the begin() here is a hidden operation. the policy for the session is specified separately so i don't want to override it if it's already there. i'll add a comment to that one

Done.

Comments from Reviewable

princjef · 2018-01-23T18:01:03Z

test/unit/test_session.js, line 25 at r2 (raw file):

Previously, princjef (Jeff Principe) wrote…

probably had something else and took it out. good catch

Done.

Comments from Reviewable

princjef force-pushed the multi-session branch 6 times, most recently from 72715a8 to 6dfc2bb Compare September 7, 2017 17:16

princjef force-pushed the multi-session branch 2 times, most recently from 9d4210f to 94f9f91 Compare September 8, 2017 02:16

princjef force-pushed the multi-session branch 3 times, most recently from 5edee8f to 0efd6b1 Compare September 19, 2017 01:24

princjef mentioned this pull request Sep 26, 2017

Event hub sometimes stops delivering messages, without errors Azure/azure-event-hubs-node#30

Closed

princjef force-pushed the multi-session branch from 0efd6b1 to 4baedbf Compare September 27, 2017 00:24

princjef force-pushed the multi-session branch 3 times, most recently from b57de35 to 75e080f Compare September 28, 2017 03:34

amarzavery closed this Jan 22, 2018

amarzavery reopened this Jan 22, 2018

pierreca reviewed Jan 23, 2018

View reviewed changes

pierreca approved these changes Jan 23, 2018

View reviewed changes

feat(session): add support for multiple sessions per client

8068045

princjef force-pushed the multi-session branch from 75e080f to 8068045 Compare January 23, 2018 17:58

pierreca merged commit da6fe5a into noodlefrenzy:master Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(session): add support for multiple sessions per client #332

feat(session): add support for multiple sessions per client #332

princjef commented Sep 7, 2017 •

edited

Loading

princjef commented Sep 7, 2017 •

edited

Loading

princjef commented Sep 8, 2017

princjef commented Sep 27, 2017

pierreca Jan 23, 2018

princjef Jan 23, 2018

pierreca Jan 23, 2018

princjef Jan 23, 2018

pierreca Jan 23, 2018

princjef Jan 23, 2018

princjef Jan 23, 2018

pierreca Jan 23, 2018

princjef Jan 23, 2018

pierreca commented Jan 23, 2018

pierreca left a comment

princjef commented Jan 23, 2018

princjef commented Jan 23, 2018

princjef commented Jan 23, 2018

feat(session): add support for multiple sessions per client #332

feat(session): add support for multiple sessions per client #332

Conversation

princjef commented Sep 7, 2017 • edited Loading

princjef commented Sep 7, 2017 • edited Loading

princjef commented Sep 8, 2017

princjef commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreca commented Jan 23, 2018

pierreca left a comment

Choose a reason for hiding this comment

princjef commented Jan 23, 2018

princjef commented Jan 23, 2018

princjef commented Jan 23, 2018

princjef commented Sep 7, 2017 •

edited

Loading

princjef commented Sep 7, 2017 •

edited

Loading