pubsub: start work on the channel api #65

reqshark · 2015-03-16T14:28:19Z

fixes #31 and alters our pubsub API, rendering it incompatible.

UPDATE (3/21/2015): API is compatible and extended:

the default sub socket will remain as it was: initialized with auto-subscription to all-channels.
socket.chan() calls internal check for the all-channels listener, removing it if necessary

New methods:

socket.chan(Array)

socket.rmchan(comma separated)

the new API lets us do stuff like:

var sub = nano.socket('sub')
sub.chan(['AAPL', 'MSFT', 'whatever'])

sub.on('message', function(msg){
  // only get msgs prefixed by those registered by .chan()
  console.log(String(msg))
})

~~Also I'd like to take advantage of NN_SUB_UNSUBSCRIBE like~~:
UPDATE (3/21/2015): We now take advantage of NN_SUB_UNSUBSCRIBE:

sub.rmchan('MSFT', 'whatever')

sub.on('message', function(msg){

  // only get msgs prefixed 'AAPL' (following from snippet above)
  console.log(String(msg))
})

~~I haven't updated tests, nor added a handler for chans given to opts object at socket initialization~~:

var nano = require('nanomsg')
var sub = nano.socket('sub', {
  chan: ['AAPL', 'MSFT', 'whatever']
})

~~I will follow up and commit those two things next if this API change meets with consensus/approval.~~

With that, let's open the forum to all opinions/arguments in favor or against this change.

nickdesaulniers · 2015-03-16T16:16:59Z

lib/index.js

+Socket.prototype.chan       = function(){
+  var args = arguments;
+  var channels = args.length;
+  while(--channels > -1) {


while (--channels) { is a common JS idiom, since 0 is false-y.

oh cool, but i think i need to include 0 here

Not if you subtract 1 from your array index, or use post decrement.

while (--i > -1) { arr[i]; }

compared to:

while (i--) { arr[i] }

reqshark · 2015-03-17T06:40:38Z

here's a detailed summary of the last commit and what we're doing for sub sockets:

updated test files with subscription sockets:

test/inproc.js
test/ipc.js
test/sockoptapi.js
test/tcp.js
test/transform.js

added test file demonstrating some of the new channel api:

test/chan.js

removed test files:

test/getsockopt.js
test/setsockopt.js

these were duplicates since documented sockopts met test coverage plus some additional.

in src/node_nanomsg.cc, removed code:

NAN_METHOD(Setsockopt)
NAN_METHOD(Getsockopt)

and simplified existing methods by dropping support for NN_SOCKET_NAME:

NAN_METHOD(Setopt)
NAN_METHOD(Getopt)

added a new method to handle subscribe/unsubscribe:

NAN_METHOD(Chan)

we're keeping socket option code restrained by separating the subscription handling concern out into its own small function. the rule of modularity asks us to do one thing and do it well.

Although socket channel registration is performed by nn_setsockopt(), @nickdesaulniers and I discussed and found shifting the operation into NAN_METHOD(Chan) reduces C pointer type complexity ordinarily arising from an encapsulated Setsockopt() method. This alternative also provides a concise and complete interface underlying API calls to socket.chan() and socket.rmchan().

with fewer tests there are exactly 800 nyan-cat, all green 🌴

nickdesaulniers · 2015-03-17T16:41:41Z

lib/index.js

+  if (opts.hasOwnProperty('chan')) {
+    if (typeof opts.chan == 'string') {
+      opts.chan.split(',').forEach(subscriber.bind(this))
+    } else {


This else branch should be an else if (Array.isArray(opts.chan)) {

OK but if that thing is neither String nor Array then I want that forEach to throw the process

nickdesaulniers · 2015-03-17T17:24:30Z

Looks good! A few minor nits, the biggest being that I think we should always expect an array of strings.

This will require a major version bump, since we're removing the getsockopt and setsockopt api, and requiring you to go through the getters/setters.

Patches that remove more code than they add 👍 👍 👍 Thanks @reqshark !

nickdesaulniers · 2015-03-17T17:26:24Z

src/node_nanomsg.cc

-    ret(NanNew<Number>(
-        nn_setsockopt(S, level, option, &optval, sizeof(optval))));
-  }
+  ret(NanNew<Number>(nn_setsockopt(S, level, option, &optval, sizeof(optval))));


make sure you run clang-format -style=Mozilla -i <file> on this!

ok will do before i update my changes.

btw, i'll try to preserve the commit history, the reason it's lost before this point is that merge conflict yesterday..

clang-format -style=Mozilla -i src/node_nanomsg.cc approved! 🚀

nickdesaulniers · 2015-03-17T17:32:51Z

Please edit your commit messages to mention that they Fixes #67, Fixes #31. Then, GH will auto close those issues when the patch is merged.

reqshark · 2015-03-17T18:23:00Z

Please edit your commit messages to mention that they Fixes #67, Fixes #31. Then, GH will auto close those issues when the patch is merged.

yep before merging, I just figured if i needed to rebase or whatever i didn't want to keep re-referencing phantom commits

nickdesaulniers · 2015-03-18T15:42:31Z

test/inproc.js

@@ -15,6 +15,8 @@ test('inproc socket pub sub', function (t) {
    var addr = 'inproc://pubsub';
    var msg = 'hello world';

+    sub.chan('');


Ah, I meant can we remove all references to the '' chan from tests that don't test filtering on a sub socket? Will the sub socket fail to work, since I think we always have to at least subscribe to the empty chan? If so, since we can have multiple subscription, then we should call this.chan('') during socket construction if it's a sub socket. Otherwise, it's silly to force users to have to explicitly subscribe to the empty channel just to use sub sockets.

can we remove all references to the '' chan from tests that don't test filtering on a sub socket?

that would be useful. maybe if we run a setTimeout or Interval after sub socket is created that checks for the existence of any length on this.channel.

If we did something like that it would make more sense to only accept arrays for topics, since we could just check length on that property.

well if you have a better implementation idea in mind I'm all ears, but the main thing is that once we've subscribed to '', there's no point in registering subsequent subscription topics, since these plus all others would get filtered into the message event callback

that would be useful. maybe if we run a setTimeout or Interval after sub socket is created that checks for the existence of any length on this.channel.

Then you run into the polling problem. What duration do you use? If too long then you miss messages, if too short then you are subject to v8/machine perf. Too brittle.

So, if you don't subscribe to any channels explicitly, sub sockets don't work (can we confirm)?

once we've subscribed to '', there's no point in registering subsequent subscription topics

Can we make '' the default, then the first time someone calls chan, can we rmchan('')?

Can we make '' the default, then the first time someone calls chan, can we rmchan('')?

excellent idea.

nickdesaulniers · 2015-03-18T15:52:34Z

lib/index.js

+  }
+
+  function subscriber(chan) {
+    if (nn.Chan(this.binding, nn.NN_SUB_SUBSCRIBE, chan) !== -1)


curly braces

reqshark · 2015-03-20T13:56:24Z

I have mixed feelings about how we're handling the default string registration and then special rmchan() stuff when that's the case. it's taking me like 30 lines of code starting from around 100 and it seems overly complicated to me... but that's the simpliest way i saw solving it...

actually maybe it's not that bad, i just took a second look after stepping away from the code...

@nickdesaulniers please review

nickdesaulniers · 2015-03-20T17:02:54Z

test/tcp.js

@@ -15,6 +15,8 @@ test('tcp socket pub sub', function (t) {
    var pub = nano.socket('pub');
    var sub = nano.socket('sub');

+    sub.chan('');
+


can be removed.

reqshark · 2015-03-22T18:25:44Z

sorry still WIP right now, I'll update with another note when it's ready for the final review..

nickdesaulniers · 2015-03-24T20:49:56Z

lib/index.js

    }
+    return this.emit('error', new Error( nn.Err() + ' : ' + chan));
+  }


Can we make register a method of the prototype?

Socket.prototype._register(chan) {};

Let's make each statement it's own line.
Let's remove return values since they're not used by any callers.

if (this.chan...) { } else if (nn.Chan) { } else {} // no explicit return statements

After this is fixed, the PR looks good to merge. Now that Travis is hooked up, I trust it enough that I don't have to pull down the patch and test it locally! 🚀 🎸

nickdesaulniers · 2015-03-24T20:53:02Z

fixes #31 and alters our pubsub API, rendering it incompatible.

While it doesn't change the pubsub API, it does remove the getsockopt and setsockopt api, and thus will require a major version bump.

nickdesaulniers · 2015-03-26T15:51:00Z

Let's focus on getting this wrapped up. There's just one last line note with a few nits, then this is good to land! 🚀 🎸

reqshark · 2015-03-26T16:16:32Z

ok let me just fix this merge conflict and add those last nits, I'll sort this out shortly

Fixes: #31 PR: #65

nickdesaulniers reviewed Mar 16, 2015
View reviewed changes

reqshark force-pushed the pubsub branch from e501d22 to 4d777da Compare March 16, 2015 19:30

reqshark mentioned this pull request Mar 16, 2015

drop support for NN_SOCKET_NAME #67

Closed

reqshark force-pushed the pubsub branch 3 times, most recently from 58f5535 to 2d41d64 Compare March 16, 2015 23:58

nickdesaulniers reviewed Mar 17, 2015
View reviewed changes

nickdesaulniers reviewed Mar 18, 2015
View reviewed changes

lib/index.js

}

function subscriber(chan) {

if (nn.Chan(this.binding, nn.NN_SUB_SUBSCRIBE, chan) !== -1)

Copy link

Owner

nickdesaulniers Mar 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curly braces

nickdesaulniers reviewed Mar 20, 2015
View reviewed changes

reqshark force-pushed the pubsub branch 2 times, most recently from b5964bd to 49713f9 Compare March 22, 2015 05:42

nickdesaulniers reviewed Mar 24, 2015
View reviewed changes

reqshark force-pushed the pubsub branch from c5b102b to f5f7efd Compare March 26, 2015 16:33

reqshark added a commit that referenced this pull request Mar 26, 2015

pubsub: the channel api

f5f7efd

Fixes: #31 PR: #65

pubsub: the channel api

9171675

Fixes: #31 PR: #65

nickdesaulniers force-pushed the pubsub branch from f5f7efd to 9171675 Compare March 27, 2015 16:57

nickdesaulniers merged commit 9171675 into master Mar 27, 2015

nickdesaulniers deleted the pubsub branch March 27, 2015 16:58

This was referenced Mar 27, 2015

Fix PUB/SUB topics support #37

Closed

1.0 release #77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pubsub: start work on the channel api #65

pubsub: start work on the channel api #65

reqshark commented Mar 16, 2015

nickdesaulniers Mar 16, 2015

reqshark Mar 16, 2015

nickdesaulniers Mar 16, 2015

reqshark Mar 16, 2015

reqshark commented Mar 17, 2015

nickdesaulniers Mar 17, 2015

reqshark Mar 17, 2015

nickdesaulniers commented Mar 17, 2015

nickdesaulniers Mar 17, 2015

reqshark Mar 17, 2015

reqshark Mar 20, 2015

nickdesaulniers commented Mar 17, 2015

reqshark commented Mar 17, 2015

nickdesaulniers Mar 18, 2015

reqshark Mar 18, 2015

nickdesaulniers Mar 18, 2015

reqshark Mar 18, 2015

nickdesaulniers Mar 18, 2015

reqshark commented Mar 20, 2015

nickdesaulniers Mar 20, 2015

reqshark commented Mar 22, 2015

nickdesaulniers Mar 24, 2015

nickdesaulniers commented Mar 24, 2015

nickdesaulniers commented Mar 26, 2015

reqshark commented Mar 26, 2015

pubsub: start work on the channel api #65

pubsub: start work on the channel api #65

Conversation

reqshark commented Mar 16, 2015

fixes #31 and alters our pubsub API, rendering it incompatible.

UPDATE (3/21/2015): API is compatible and extended:

socket.chan(Array)

socket.rmchan(comma separated)

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

reqshark commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Mar 17, 2015

reqshark commented Mar 17, 2015

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

reqshark commented Mar 20, 2015

Choose a reason for hiding this comment

reqshark commented Mar 22, 2015

Choose a reason for hiding this comment

nickdesaulniers commented Mar 24, 2015

nickdesaulniers commented Mar 26, 2015

reqshark commented Mar 26, 2015