Http transport by pelger · Pull Request #74 · apparatus/mu

pelger · 2016-12-14T13:11:12Z

No description provided.

coveralls · 2016-12-14T13:19:22Z

Coverage remained the same at 100.0% when pulling 281c2ac on http_transport into 2955d32 on master.

mcollina · 2016-12-14T16:27:01Z

packages/mu-http/driver.js

+          sendTo.response.write(stringify(message))
+          sendTo.response.end()
+        } else {
+          cb && cb(mue.transport('connection has alredy recieved response!'))


what if there is no callback?

Then it fails silently of course... :) actually that check was put in place whilst I was working on the last merge - will remove it and assume that the callback is always there this is a safe assumption as driver is an internal thing. Without the check it will blow up with no callback which is the correct behaviour

mcollina · 2016-12-14T16:28:20Z

packages/mu-http/driver.js

+                                port: options.target.port,
+                                method: 'POST',
+                                path: '/',
+                                headers: {'Content-Type': 'application/json'}})


You should use an Agent here to speed things up. creating a new TCP connection for each things.

agreed - will update

mcollina · 2016-12-14T16:28:37Z

packages/mu-http/driver.js

+        var req = http.request({host: options.target.host,
+                                port: options.target.port,
+                                method: 'POST',
+                                path: '/',


probably a good idea to use a mu-specific path.

fair enough - /mu/ ?

mcollina · 2016-12-14T16:29:04Z

packages/mu-http/driver.js

+
+        req.on('error', function (err) {
+          cb(mue.transport(err))
+        })


we should try to avoid some of those closures for speed reasons.

yep - I'll leave one for later - premature optimization and all that...

mcollina · 2016-12-14T16:29:43Z

packages/mu-http/driver.js

+        if (cb) {
+          setImmediate(function () {
+            server.close(cb)
+          })


why the setImmediate wrap?

Again one for @davidmarkclements - I assume the intent is to allow existing I/O to drain - but probably not needed...

I can't remember why - but this is definitely needed - at least in the tcp transport.

after destroying the connections, and immediately closing I think it goes into a zombie state and keeps the port open - the setImmediate solved this

the problem was hard to reproduce, it was intermittent, but the tests would sometimes fail because of it

Stick a comment in there. Plus, it's a code smell that something is very wrong in the tear down sequence. It should not be needed.

good point - also worth digging into this to understand fully - most likely we could expose this condition with some load testing

mcollina · 2016-12-14T16:35:32Z

packages/mu-http/driver.js

+            connectionsByIp[request.socket.remoteAddress + '_' + request.socket.remotePort] = []
+          }
+          connections[inbound.value.protocol.src].push({request: request, response: response})
+          connectionsByIp[request.socket.remoteAddress + '_' + request.socket.remotePort] = inbound.value.protocol.src


why are we doing this? an http request is not a socket, and it can't be reuse.

also, we should probably use http://npm.im/end-of-stream to handle all possible cases (including errors)

Because that connection is written to later in the pattern handling sequence and then removed. https://github.com/apparatus/mu/blob/http_transport/packages/mu-http/driver.js#L41-L45

oooh, now I understand. perfect.

mcollina · 2016-12-14T16:42:02Z

packages/mu-http/driver.js

+    server = options.source && listen(options.source.port, options.source.host, options.ready)
+
+    if (!server && options.ready instanceof Function) {
+      options.ready()


why we ended up having ready()? I thought we had no callbacks at all for transports, services, etc.

One for @davidmarkclements I think :)

Memory is hazy on this, but there was definitely a good reason, I think it was needed for tests (or rather to avoid using setTimeouts in tests) - and may also be needed internally

If we need to have a ready() thing, we can just have a full-blown plugin system like boot-in-the-arse and be done with it. Ready callbacks are tricky when there are multiple ones to be started.

nooooo plugins!!!!! let me review the whole ready thing

So whilst this is not generally needed, there may be edge cases when a signal back from the underlying driver to indicate that it is ready may be useful. I suggest that we keep this in for now and review after some more field testing if unused.

@pelger I think it is needed. And if it is needed, you are off with something like https://github.com/mcollina/boot-in-the-arse rather than cooking up something internal.
If it is not needed, we are better off without! :)

davidmarkclements

overall awesomeness - just a couple of requests

davidmarkclements · 2016-12-15T09:09:30Z

packages/mu-http/driver.js

+
+            inbound = parse(Buffer.concat(body))
+            receive(inbound.err, inbound.value)
+          })


what if the response stream emits an error or closes unexpectedly - maybe end-of-stream needed here

parse will fail and inbound.err will be set.

davidmarkclements · 2016-12-15T09:13:03Z

packages/mu-http/driver.js

+                                headers: {'Content-Type': 'application/json'}})
+
+        req.on('response', function (response) {
+          response.on('data', function (chunk) {


on a general note, I'm concerned we're missing the ability to stream from transport to user land (c.f. #46)

yes there has to of course be a buffering mode (which is our only option), but for large data blobs being able to pattern match against a stream and/or associate a pattern to a stream across every transport would remove all limitations

This would be good - suggest that we merge this in and then add this capability to the roadmap - would like to think this through a little!

davidmarkclements · 2016-12-15T09:13:47Z

packages/mu-http/driver.js

+        })
+
+        req.write(stringify(message))
+        req.end()


you can just do req.end(stringify(message))

davidmarkclements · 2016-12-15T09:16:45Z

packages/mu-http/driver.js

+
+
+    function listen (port, host, ready) {
+      server = http.createServer()


principle of least surprise would encourage assignment to variables as early as possible,
in the tcp transport listen returns the server object, and at the top, server is assigned to the result of listen - could we follow the same approach here (and all transports) ?

realise listen returns server - no need to assign to server var here

davidmarkclements · 2016-12-15T09:19:20Z

packages/mu-http/driver.js

+ * HTTP transport
+ * Figure out path mapping - perhaps this may need to be part of the config for now just /
+ */
+module.exports = function createTcpDriver (options) {


function should be named createHttpDriver

Opps yep my bad!

davidmarkclements · 2016-12-15T09:21:28Z

packages/mu-http/driver.js

+    function send (message, cb) {
+      var sendTo
+
+      if (connections[message.protocol.dst]) {


suggest breaking the two main cases into two functions - primarily for readability - but also makes inlining more likely

davidmarkclements · 2016-12-15T09:22:31Z

packages/mu-http/driver.js

+
+
+
+    server = options.source && listen(options.source.port, options.source.host, options.ready)


this should be at the top - see previous comments

davidmarkclements · 2016-12-15T09:23:49Z

packages/mu-transport/index.js

-      return
+      if (!msg || !msg.protocol) {
+        return
+      } else {


no need for else after early return

Ermmm... you want to just re-read that code Dave :)

I re-read... I'm seeing a return and then an else - the else is redundant - what am I missing?

davidmarkclements · 2016-12-15T09:25:33Z

packages/mu-transport/index.js

+                protocol: {}}
+    var elt = muid
+
+    /* istanbul ignore else  */


not keen on artificially maintaining coverage

question is, if we have to do this, maybe the checks aren't necessary

if they are, then we should test (and handle) the alternatives

Agreed - but IMO the occasional pragma to maintain 100% is worth it - branch coverage is sometimes too fiddly :)

I submit that its worth the investment for core - the fiddling can reveal things not thought of, or if not it gives confidence that the logic is sound and cases well covered

Fair comment - however this is 1/2 a days worth so I'll put it on the backlog for now!

Please remove that sentence, so we do not forget about it. It's ok if coverages drops consequently.

davidmarkclements · 2016-12-15T09:27:39Z

packages/mu-transport/index.js

+      if (!msg || !msg.protocol) {
+        return
+      } else {
+        msg = constructFailResponse(err, msg)


this is much better than before

coveralls · 2017-01-09T13:33:59Z

Coverage remained the same at 100.0% when pulling 7915805 on http_transport into 2955d32 on master.

coveralls · 2017-01-09T13:38:07Z

Coverage remained the same at 100.0% when pulling 7915805 on http_transport into 2955d32 on master.

mcollina

Some nits should be fixed.

mcollina · 2017-01-09T13:54:06Z

packages/mu-http/driver.js

+    server = options.source && listen(options.source.port, options.source.host, options.ready)
+
+    if (!server && options.ready instanceof Function) {
+      options.ready()


@pelger I think it is needed. And if it is needed, you are off with something like https://github.com/mcollina/boot-in-the-arse rather than cooking up something internal.
If it is not needed, we are better off without! :)

mcollina · 2017-01-09T13:54:21Z

packages/mu-http/driver.js

+    function send (message, cb) {
+      var sendTo
+
+      if (connections[message.protocol.dst]) {


mcollina · 2017-01-09T13:55:40Z

packages/mu-http/driver.js

+        })
+
+        request.on('end', function () {
+          inbound = parse(Buffer.concat(body))


It is way better to use strings here, e.g.:

var body = '' request.setEncoding('utf8') request.on('data', (chunk) => body += chunk)

Is it better to use strings or to concat actual buffers? (instead of using array)

mcollina · 2017-01-09T13:56:37Z

packages/mu-http/driver.js

+        request.on('end', function () {
+          inbound = parse(Buffer.concat(body))
+
+          if (!connections[inbound.value.protocol.src]) {


you should protect for inbound.value.protocol.src to exist before proceeding.

mcollina · 2017-01-09T13:57:38Z

packages/mu-transport/index.js

+                protocol: {}}
+    var elt = muid
+
+    /* istanbul ignore else  */


Please remove that sentence, so we do not forget about it. It's ok if coverages drops consequently.

mcollina · 2017-01-09T13:58:13Z

test/drivers/http-driver/500error.test.js

+
+    request.on('data', function (chunk) {
+      body.push(chunk)
+    })


are you doing anything with body here?

coveralls · 2017-02-21T17:15:12Z

Coverage remained the same at 100.0% when pulling 4591708 on http_transport into 2955d32 on master.

pelger added 2 commits December 14, 2016 11:19

added http driver and protocol error tests

2c9c726

merge and coverage

281c2ac

pelger requested review from davidmarkclements and mcollina December 14, 2016 13:11

pelger mentioned this pull request Dec 14, 2016

Roadmap #68

Open

5 tasks

mcollina reviewed Dec 14, 2016

View reviewed changes

mcollina requested changes Dec 14, 2016

View reviewed changes

pending changes PR review MC

d06797b

davidmarkclements requested changes Dec 15, 2016

View reviewed changes

updates from review

7915805

mcollina mentioned this pull request Jan 9, 2017

Dns adapter #75

Merged

mcollina requested changes Jan 9, 2017

View reviewed changes

tweaks from review

4591708

pelger merged commit 5697128 into master Feb 21, 2017

pelger deleted the http_transport branch February 21, 2017 17:11



		function listen (port, host, ready) {
		server = http.createServer()




		server = options.source && listen(options.source.port, options.source.host, options.ready)

Conversation

pelger commented Dec 14, 2016

Uh oh!

coveralls commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmarkclements Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmarkclements left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Dec 14, 2016 •

edited

Loading

davidmarkclements Dec 15, 2016 •

edited

Loading