1098017 - socket: Prototype simple json+unix socket replacement for libzmq

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[James Lal [:lightsofapollo]]

zmqp is strictly better but we have a few issues:

 - requires libzmq 
 - we have some trouble getting this into tbpl
 - we are only using one feature of libzmq

I am hugely motivated to get a PoC so we get the benefits of crash reporting, etc.. and can ship that quickly (early next week) on TBPL without modifying the testers.

Comment 1

[James Lal [:lightsofapollo]]

Attachment: https://github.com/mozilla-b2g/marionette-socket-host/pull/12

This was based off other patches so will land/address comments in those first which will make this a bit easier to review... This should be ready for testing though... I will begin /w b2g desktop and then play /w my flame

Comment 2

[James Lal [:lightsofapollo]]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

Still somewhat buggy ... going to introduce promises (instead of callbacks) and add some guards for common races .

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

f+ but I'd like to see the server spin up a thread per request so it can handle multiple requests. May be useful depending on the frequency of emitted requests, or if we have multiple connections

Comment 4

[James Lal [:lightsofapollo]]

This is a good idea I will refactor to use https://docs.python.org/2/library/socketserver.html#examples in my next patch... its needed to handle SIGINT correctly anyway.

Comment 5

[Andrew Halberstadt [:ahal]]

This bug makes me sad, but I understand where you guys are coming from. Sorry for causing headaches, at the time I thought I'd be the one dealing with it.

Out of curiosity, what exactly was causing problems? Deployment on slaves, local environemnts, or both? Eventually I'd love zmq to become a standard library included by default on any test/build slaves or docker images that mozilla creates. Maybe that's being too ambitious.

Comment 6

[James Lal [:lightsofapollo]]

There are a few roadblocks we hit (none of these are impossible to overcome)

 - getting everything working nicely on test slaves (https://bugzilla.mozilla.org/show_bug.cgi?id=1089712)
 - local setups requiring libzmq installed locally 

>Out of curiosity, what exactly was causing problems? Deployment on slaves, local environemnts, or both? >Eventually I'd love zmq to become a standard library included by default on any test/build slaves or >docker images that mozilla creates. Maybe that's being too ambitious.

I don't think it's too ambitious my motivation here is purely we need something quickly and the unix socket approach is both quick and requires no changes to testers/local setups.

The underlying protocol is the same thing we use in marionette (<byte length>:<json>) so its not something totally unknown. Technically speaking I agree zmq is a better basis for machine <-> machine interactions in this case though I don't think we are winning anything by using it (in the short-mid term anyway).

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to James Lal [:lightsofapollo] from comment #6) 
> Technically speaking I
> agree zmq is a better basis for machine <-> machine interactions in this
> case though I don't think we are winning anything by using it (in the
> short-mid term anyway).

Agreed. I had some long term visions of it being used in more places in the future, but that shouldn't get in the way of short term productivity. Sorry for the trouble this caused.

Comment 8

[James Lal [:lightsofapollo]]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

Okay- new version with mulit-threaded server and some better erorr handling... I am testing this on device now (desktop tests pass well) so might be slightly early to review but it might be worth looking at the code.

Comment 9

[James Lal [:lightsofapollo]]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

This also handles https://bugzilla.mozilla.org/show_bug.cgi?id=1058796 (a by product of the http/error handling changes). Any errors during startup/teardown are sent to the JS process.

Note: I have only lightly tested device cases as of Today (my time Sunday) by Monday should have fixed any remaining device issues but be on the lookout for any breakages, changes there.

Comment 10

[Zac C (:zac)]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

with regard to 1058796, it does stop it hanging although it still builds the profile and tries to execute the tests. Potentially it could fail/exit even earlier than this, but it's probably not a blocker.

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8521897 [details] [review]
https://github.com/mozilla-b2g/marionette-socket-host/pull/12

r+ though I'm not sure what the benefit is using HTTP over pure json over socket.

Also, if I start with my device unplugged or if I unplug my device while the tests are running, I don't see Bug 1058796 being handled. Instead each test fails with:

TEST-UNEXPECTED-FAIL | apps/communications/contacts/test/marionette/search_test.js | Contacts > Search "after all" hook
TypeError: Cannot call method 'stop' of undefined
    at Context.<anonymous> (/Users/mdas/Code/gaia/node_modules/marionette-js-runner/lib/runtime/hostmanager.js:112:12)
    at Hook.Runnable.run (/Users/mdas/Code/gaia/node_modules/mocha/lib/runnable.js:196:15)
    at next (/Users/mdas/Code/gaia/node_modules/mocha/lib/runner.js:258:10)
    at Object._onImmediate (/Users/mdas/Code/gaia/node_modules/mocha/lib/runner.js:275:5)
    at processImmediate [as _immediateCallback] (timers.js:345:15)

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

oh and I have nits in the PR

Comment 13

[James Lal [:lightsofapollo]]

Nits addressed- the logic _could_ be faster (and I intend to keep making it faster) but to detect failure in the general case only takes:

./node_modules/.bin/marionette-mocha --buildapp=device   0.33s user 0.07s system 67% cpu 0.578 total

Which is fine by me

Comment 14

[James Lal [:lightsofapollo]]

https://github.com/mozilla-b2g/marionette-socket-host/commit/2190249528eef565db7112f4bb27c20cc319c9ab