sockopt API #46

reqshark · 2015-02-20T06:52:05Z

• convenience methods for get/setsockopt()

an interface to nanomsg socket options by way of:

socket.option(param) or socket.option()

passing a param sets that, while no param gets that.

*a few caveats will be referenced in forthcoming docs

it’s the simple interface i wrote in nanomsg.iojs covering:

• tcpnodelay
• linger
• sndbuf
• rcvbuf
• sndtimeo
• rcvtimeo
• reconn
• maxreconn
• sndprio
• rcvprio

cheers! here are the docs for this change: https://github.com/reqshark/nanomsg.iojs#sockettcpnodelayboolean

should we add those docs directly into the readme or maybe create a docs directory? I think there's going to be more documentation to follow this...

please comment and criticize the coding style: in the C/C++ I left Getsockopt() and Setsockopt() unchanged because i thought we should consider the differences and maybe merge them into something singular somehow.. also i dont want to break any tests yet!

reqshark · 2015-02-20T16:47:35Z

lib/index.js

+  sndprio         : nn.NN_SNDPRIO,
+  tcpnodelay      : nn.NN_TCP_NODELAY
+}
+


as far as style goes, should the var sol declaration be hoisted to the top of the file? btw i called it sol since it was named after socket level: NN_SOL_SOCKET

I'm fine without explicit hoisting. Maybe a comment pointing people to the man page to describe the socket options?

reqshark · 2015-02-20T17:02:30Z

with these functions in place on the this value of the socket class, (alternatively you could use module.exports and the prototype declaration, etc.), but bear with me pls, why i'd like to attach them onthis is so that we can pass socket options at initialization, like:

nano.socket(type, [options,])

var nano = require('nanomsg')

var sub = nano.socket('sub', {
  tcpnodelay: true,
  rcvbuf: 1024
})

to get there from here, now we could just one-liner the opts check inside our javascript socket class like:

for(var sokopt in sol) if(opts.hasOwnProperty(sokopt)) this[sokopt](opts[sokopt])

this[sokopt](opts[sokopt]) initializes any sockopt function with param passed by opts

if we do that then var sol object needs to be hoisted above the Socket function declaration

nickdesaulniers · 2015-02-20T17:56:02Z

lib/index.js

+     this.maxreconn  = maxreconn
+     this.sndprio    = sndprio
+     this.rcvprio    = rcvprio
+     this.tcpnodelay = tcpnodelay


I'll have to be more stringent about style throughout the code base, but please add semicolons.

i guess... but isn't it kinda spicy without them?

maybe, I'm looking to use something like semistandard though so can just write code however, and run the formatter during the tests.

Also, should these be = sol.linger; ...?

it's cool i'll add them, not using them makes me forget them when i switch to C/C++ sometimes anyway

nickdesaulniers · 2015-02-20T18:07:45Z

While the syntax is certainly uglier for us, what do you think about making the methods getters/setters?

for(var sokopt in sol) if(opts.hasOwnProperty(sokopt)) this[sokopt](opts[sokopt])

would become

for(var sokopt in sol) if(opts.hasOwnProperty(sokopt)) this[sokopt] = opts[sokopt];

nickdesaulniers · 2015-02-20T18:08:41Z

should we add those docs directly into the readme or maybe create a docs directory? I think there's going to be more documentation to follow this...

Let's put everything in the readme. Want to add it to this PR, or make a followup issue?

reqshark · 2015-02-20T18:15:23Z

Let's put everything in the readme. Want to add it to this PR, or make a followup issue?

i'll add that in a few minutes, with the rest of the changes from the comments

reqshark · 2015-02-20T18:17:50Z

so can we do the opts initialization API? like:

nano.socket(type, [options,])

where we pass opts that call respective setsockopt()methods?

nickdesaulniers · 2015-02-20T18:19:32Z

so can we do the opts initialization API? like:

It's a good idea. Just do it ™️

reqshark · 2015-02-20T18:32:32Z

While the syntax is certainly uglier for us, what do you think about making the methods getters/setters?
this[sokopt] = opts[sokopt];

the point of that one-liner is just to call the respective sockopt setter.. firing a function call. i'm updating this and the comments, you'll see after I push the initializer stuff, I'll keep that in mind tho

nickdesaulniers · 2015-02-20T18:35:33Z

lib/index.js

+        throw new Error(nn.Err() + ': '+this.type+' nodelay@'+'getsockopt'+'\n')
+    }
+  }
+}


Why does this function return a string of some custom format and not a bool? Just make it explicit in the docs whether true/false means on/off.

Oh, just saw all of these functions do. Why not just return the value? It should be up to the consumer of the lib to log as they see fit.

Why does this function return a string of some custom format and not a bool? Just make it explicit in the docs whether true/false means on/off.

wow good call

nice article btw, here's a quote on topic from Ken Arnold:

I think that the terseness of Unix programs is a central feature of the style. When your program's output becomes another's input, it should be easy to pick out the needed bits. And for people it is a human-factors necessity — important information should not be mixed in with verbosity about internal program behavior. If all displayed information is important, important information is easy to find.

reqshark · 2015-02-20T22:15:22Z

just added the options initializer, so we can set any of those options as we create the socket.

try it!

var pub = nano.socket('pub')
var sub = nano.socket('sub', { tcpnodelay: true } )

console.log(pub.tcpnodelay()) //false
console.log(sub.tcpnodelay()) //true

also added some api docs to the readme.. just note that all the links to further down the document in the nano.socket(type, [options,]) bullet points are linked based on the idea that the readme resides on master and not this branch

nickdesaulniers · 2015-02-20T22:22:15Z

lib/index.js

@@ -290,7 +326,7 @@ util.inherits(Device, EventEmitter);
 */
 function createSocket (type, opts) {
    var domain = (opts || {}).raw ? nn.AF_SP_RAW : nn.AF_SP;
-    return new Socket(domain, type);
+    return new Socket(domain, type, opts);


delete var domain ... and the first arg. No need to pull it out of opts here, since we pass on opts. Fix up the Socket constructor too to pull the domain out of opts.

ya i was doing that but for some reason the transform.js test would time out.. tape would just stall forever wtf so i just left domain untouched.

I'll restart my machine and pull down a fresh copy, not sure why domain opt would have anything to do with that test timing out..

From simply removing a constructor argument? You didn't rename the property did you? see https://github.com/nickdesaulniers/node-nanomsg/blob/master/lib/index.js#L11

ya probably, sure.

we definitely need to just pass opts, domain included, it's very ugly without doing that

we dont have a domain param or var anymore so i got rid of the warning

reqshark · 2015-02-21T00:49:32Z

That Number check in JS is extra work for the JIT. No question. It will add up if u have to run a check on all inbound recv msgs.

If we adopt streams next, the Number type will cause an exception in the Readable base class, if you're in the middle of a buffer stream, and probably even object mode too

reqshark · 2015-02-21T02:11:59Z

can I check in my Makefile, take it off .gitignore list? Is there a reason it's being .gitignored?

the one i use eases build/test across different versions of node and OSs:

.PHONY: clean check test perf bench full

ALL:
    npm i

check:
    npm t

test:
    npm t

clean:
    rm -fr build && rm -rf node_modules

perf:
    node perf/local_lat.js tcp://127.0.0.1:5555 1 100000& node perf/remote_lat.js tcp://127.0.0.1:5555 1 100000
    node perf/local_thr.js tcp://127.0.0.1:5556 1 100000& node perf/remote_thr.js tcp://127.0.0.1:5556 1 100000

bench:
    node perf/local_lat.js tcp://127.0.0.1:5555 10 1000& node perf/remote_lat.js tcp://127.0.0.1:5555 10 1000
    node perf/local_thr.js tcp://127.0.0.1:5556 10 100000& node perf/remote_thr.js tcp://127.0.0.1:5556 10 100000

full:
    rm -fr build && rm -rf node_modules
    npm i && npm t

reqshark · 2015-02-21T21:24:37Z

yo @nickdesaulniers can I check my Makefile into this repo and take Makefile off .gitnore?

EDIT: nevermind i just did

reqshark · 2015-02-21T22:47:36Z

updates to commit 9fa3300:

These should be added on the prototype, no need to have unique copies per Socket instance.

done.

I wonder what it would look like without call

behold.

you could probably switch the order of the arguments

done.

nickdesaulniers · 2015-02-23T17:28:57Z

lib/index.js

+  reconn          : nn.NN_RECONNECT_IVL,
+  maxreconn       : nn.NN_RECONNECT_IVL_MAX,
+  sndprio         : nn.NN_SNDPRIO,
+  rcvprio         : 9,


I'm not sure how I feel about committing this. I'd like to have a test case that shows it's broken and a comment

// TODO: Issue #50

nickdesaulniers · 2015-02-23T17:35:08Z

This is turning out to be a great addition so far! Thanks for all of your hard work, Bent!

reqshark · 2015-02-23T19:28:45Z

haha thanks for that, my pleasure, Nick! Just saw your latest comments, i'll fix/update those this evening

reqshark · 2015-02-24T01:59:46Z

@nickdesaulniers I think that should just about do it, anything else?

• convenience methods for `get/setsockopt()`, an interface to nanomsg socket options by way of: `socket.option(param)` or `socket.option()` passing a param sets that, while no param gets that. it’s the simple interface i wrote in nanomsg.iojs covering: - tcpnodelay - linger - sndbuf - rcvbuf - sndtimeo - rcvtimeo - reconn - maxreconn - sndprio - rcvprio • adds documentation • fixes #47 with a necessary check removed in c0336f1 • if nn_recv() errors setting len to -1, node buffer library will accordingly refuse to allocate (malloc or smalloc) an impossible size in memory • test: set sockopts when starting the socket • unorthodox symbol fix: rvcprio() • the Makefile • adds TODO: Issue #50 comment on line 19 • rewrite opts check for RAW & remove redundant error check - RAW option now more consistent with its description in docs • partial function application: tighten up opts API methods • test: RCVPRIO, ensure symbol test fails for the missing symbol • bump version PR: #46

following up a discussion we had about getsockopt in #46

reqshark reviewed Feb 20, 2015
View reviewed changes

nickdesaulniers reviewed Feb 20, 2015
View reviewed changes

nickdesaulniers mentioned this pull request Feb 20, 2015

buffer error: kMaxLength #47

Closed

nickdesaulniers reviewed Feb 20, 2015
View reviewed changes

nickdesaulniers reviewed Feb 23, 2015
View reviewed changes

nickdesaulniers mentioned this pull request Feb 24, 2015

getopt broken for nn_sub #51

Closed

nickdesaulniers closed this Feb 24, 2015

nickdesaulniers force-pushed the sockopts branch from d9136bd to 5fa3305 Compare February 24, 2015 18:46

reqshark deleted the sockopts branch February 24, 2015 18:48

reqshark added a commit that referenced this pull request Feb 27, 2015

optval pointer fix

38bd0a7

following up a discussion we had about getsockopt in #46

reqshark mentioned this pull request Feb 27, 2015

optval pointer fix #61

Closed

reqshark mentioned this pull request Feb 23, 2016

tests that prove new socket options work #52

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sockopt API #46

sockopt API #46

reqshark commented Feb 20, 2015

reqshark Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark commented Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark Feb 20, 2015

nickdesaulniers Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark Feb 20, 2015

nickdesaulniers commented Feb 20, 2015

nickdesaulniers commented Feb 20, 2015

reqshark commented Feb 20, 2015

reqshark commented Feb 20, 2015

nickdesaulniers commented Feb 20, 2015

reqshark commented Feb 20, 2015

nickdesaulniers Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark Feb 20, 2015

reqshark commented Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark Feb 20, 2015

nickdesaulniers Feb 20, 2015

reqshark Feb 20, 2015

reqshark Feb 20, 2015

reqshark commented Feb 21, 2015

reqshark commented Feb 21, 2015

reqshark commented Feb 21, 2015

reqshark commented Feb 21, 2015

nickdesaulniers Feb 23, 2015

nickdesaulniers commented Feb 23, 2015

reqshark commented Feb 23, 2015

reqshark commented Feb 24, 2015

sockopt API #46

sockopt API #46

Conversation

reqshark commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reqshark commented Feb 20, 2015

nano.socket(type, [options,])

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

nickdesaulniers commented Feb 20, 2015

nickdesaulniers commented Feb 20, 2015

reqshark commented Feb 20, 2015

reqshark commented Feb 20, 2015

nano.socket(type, [options,])

nickdesaulniers commented Feb 20, 2015

reqshark commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reqshark commented Feb 20, 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 Feb 21, 2015

reqshark commented Feb 21, 2015

reqshark commented Feb 21, 2015

reqshark commented Feb 21, 2015

Choose a reason for hiding this comment

nickdesaulniers commented Feb 23, 2015

reqshark commented Feb 23, 2015

reqshark commented Feb 24, 2015