noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

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

Closed princjef closed 6 years ago

princjef commented 7 years ago

Addresses https://github.com/noodlefrenzy/node-amqp10/issues/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

Tests

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 Reviewable

princjef commented 7 years ago

I also pulled in the change from https://github.com/noodlefrenzy/node-amqp10/pull/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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

reestablish? the spec talks about establishing and ending sessions


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

princjef commented 6 years ago

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


Comments from Reviewable

princjef commented 6 years ago

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 commented 6 years ago

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