Skip to content
This repository has been archived by the owner on Mar 21, 2018. It is now read-only.

Always launch a new process for the UA service. #516

Merged
merged 1 commit into from May 26, 2016
Merged

Conversation

Mossop
Copy link
Member

@Mossop Mossop commented May 24, 2016

This doesn't work yet (the UA service doesn't start up) but it should be pretty close. Basically we try to connect the websocket, if it fails we start a new service and connect again. The service closes itself once all clients have gone away.

Does this seem like it is along the right lines @ncalexan?

One issue is that there is a delay between starting the service and being able to connect. I almost want to make the main process listen for a connection from the service in that case.

const timeout = (ms) => new Promise(resolve => setTimeout(resolve, ms));

function startService() {
// Runs Electron as a node process
Copy link
Member

@ncalexan ncalexan May 24, 2016

Choose a reason for hiding this comment

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

Is there an advantage to launching Electron in this way? Should we perhaps launch argv[0] or something to respawn? I'd quite like to not bring Electron into the UA service at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the one thing we know a tofino user has installed is the current electron build. They may not have node or anything else unless we add that as a dependency and ship it.

@ncalexan
Copy link
Member

This looks mostly okay to me. I've left a few nits and comments throughout; interested to see the final form.

@Mossop
Copy link
Member Author

Mossop commented May 25, 2016

I've updated this and it is ready for review though there are no tests right now. If you have suggestions beyond the webdriver tests verifying this as a side effect then let me know.

At some point it might make sense to merge shared/user-agent-client.js and ui/browser/lib/user-agent.js, but by the time I discovered that I was almost done so I'd rather get this landed first and see if any issues come up.

@coveralls
Copy link

coveralls commented May 25, 2016

Coverage Status

Coverage decreased (-0.5%) to 41.613% when pulling 63e4dd4 on mossop/newprocess into 403045b on master.

}

async function backoffConnect() {
let timedout = false;
Copy link
Member

Choose a reason for hiding this comment

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

nit: timedOut.

@ncalexan
Copy link
Member

Nifty! This works well locally, as long as I don't kill the initiating process. (That's an intentional limitation.)

Throughout, I prefer full sentence comments, but whatever.

@ncalexan ncalexan assigned Mossop and unassigned ncalexan May 26, 2016
@Mossop
Copy link
Member Author

Mossop commented May 26, 2016

Really? I can kill the initiating process on windows and it seemed to work last I checked.

import path from 'path';
import EventEmitter from 'events';
import WebSocket from 'ws';
import request from 'request';
Copy link
Contributor

Choose a reason for hiding this comment

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

We have isomorphic-fetch, do we also need request? We should have one or the other

Signed-off-by: Dave Townsend <dtownsend@oxymoronical.com>
@Mossop Mossop merged commit beaf6c3 into master May 26, 2016
@Mossop Mossop deleted the mossop/newprocess branch May 26, 2016 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants