Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Electron: migrate to Shadowsocks Go #688

Open
wants to merge 12 commits into
base: alalama-go-tun2socks
from

Conversation

@alalamav
Copy link
Contributor

alalamav commented Oct 18, 2019

  • Migrates the Windows and Linux clients to Shadowsocks Go, provided by an outline-go-tun2socks binary.
  • Deprecates the typescript connectivity checks.
  • Removes shadowsocks-libev.
  • Updates the DNS resolvers to include Cloudflare and Quad9 (removes Dyn for lack of TCP support).
@alalamav alalamav requested a review from bemasc Oct 18, 2019
@alalamav alalamav self-assigned this Oct 18, 2019
// -tunAddr 10.0.85.2 -tunGw 10.0.85.1 -tunMask 255.255.255.0 \
// -proxyServer 127.0.0.1:1080 -disableDNSCache -dnsFallback
// -proxyHost 127.0.0.1 -proxyPort 1080 -proxyPassword mypassword \
// -proxyCipher chacha20-ietf-poly1035 -checkConnectivity

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

Nit: how about [-checkConnectivity] to indicate that it's optional.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

Done.

};
});

return Promise.race([running, earlyExit]);

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

I think this can be implemented without Promise.race, by returning a single promise that registers listeners for both resolve and reject.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

Done.

if (exitListener) {
exitListener(); // Execute previous exit listener
}
reject(errors.fromErrorCode(code || errors.ErrorCode.UNEXPECTED));

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

What if code is 0? In that case, I think this relies on onStdout running beforeonExit, which is likely safe but slightly scary. I think an explicit type check would be clearer.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

code === 0 should not happen unless we invoked tun2socks with -version, which we don't. I think we should treat it like an unexpected error. Added a comment to explain.

@@ -205,33 +177,19 @@ export class ConnectionManager {
console.log('restarting tun2socks after resume');

this.tun2socks.onExit = this.tun2socksExitListener;
this.tun2socks.start(this.isUdpEnabled);
this.tun2socks.start(!this.isAutoConnect);

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

This is scary: this code kicks off an async start of tun2socks, and then attempts to restart it without waiting for it to start. It seems hard to be sure that this doesn't have any race conditions. Could this call use await instead?

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

This was a mistake on my part. We should only (re)start tun2socks once and leverage the UDP check. The routing service sends a 'reconnected' event on resume, after the network is re-established. To avoid a race condition with the network change listener, this method now only restores tun2socks' exit listener.

this.launch(args);

const exitListener = this.exitListener;

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

I think this pattern of replacing the listener creates a lot of potential confusion. For example, the onExit setter only works when called before start: subsequent changes are silently ignored.

I wonder if this could be simplified by removing ChildProcessHelper, now that there's only one subclass. Alternatively, Tun2socks could own a ChildProcessHelper instead of subclassing it, to avoid having to change its own public fields.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

Per our offline conversation, I have encapsulated tun2socks' restart behavior in the Tun2socks class. To this end, I followed your suggestion to have Tun2socksown a ChildProcessHelper. PTAL: b946211.

@@ -178,7 +150,7 @@ export class ConnectionManager {

// Test whether UDP availability has changed; since it won't change 99% of the time, do this

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

This comment might still be accurate but I think it's worth rephrasing to reflect the new design.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

Done.

@@ -96,8 +80,7 @@ async function isSsLocalReachable() {
// - silently restart tun2socks when the system is about to suspend (Windows only)

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 21, 2019

Contributor

This comment (and the corresponding log message in suspendListener) seems to be inaccurate. Instead, it seems like the code silently restarts tun2socks after the system resumes. Do we know if this is actually necessary? It seems like it would be simpler if we could skip that restart (which, together with the UDP check, creates a double-restart).

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 23, 2019

Author Contributor

Per my comment above, we now only (re)start tun2socks once (no extra start for UDP). Unfortunately, it is necessary to restart tun2socks on resume because the process terminates on suspend due to the TAP device getting closed.

I have updated the comment to accurately reflect this behavior.

// lifecycle:
// - once any helper fails or exits, stop them all
// - once *all* helpers have stopped, we're done
const exits = [
this.routing.onceDisconnected, new Promise<void>((fulfill) => this.ssLocal.onExit = fulfill),
this.routing.onceDisconnected,
new Promise<void>((fulfill) => {

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

I think onTun2socksExit could be removed if this Promise is instantiated before this.tun2socks. Alternatively, Tun2socks could expose a onceClosed promise like RoutingDaemon.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Followed the suggestion to expose a promise.


private exitListener?: () => void;
protected exitListener?: (code?: number, signal?: string) => void;

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

Nothing extends ChildProcessHelper, so these can be private, right?

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Done.


const onExit = () => {
const onExit = (code?: number, signal?: string) => {
this.running = false;
if (this.process) {
this.process.removeAllListeners();

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

Are you sure that this is needed? In theory, the garbage collector should be able to handle this on its own.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Yes, otherwise we'd register duplicate listeners on re-launch.

this.process.onStdout = undefined;
this.process.onExit = this.exitListener;
if (isWindows) {
powerMonitor.on('suspend', this.suspendListener.bind(this));

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

It looks like these listeners both remove themselves, so powerMonitor.once seems like an easier solution.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Done. I had tried that, but it wasn't firing only once. It now works with your suggestion to make the listeners instance variables.

super(pathToEmbeddedBinary('go-tun2socks', 'tun2socks'));
stop() {
if (isWindows) {
powerMonitor.removeListener('suspend', this.suspendListener.bind(this));

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

I would expect each call to bind to produce a new closure object, so I would be surprised if this call actually removes the existing listener.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

I saw some strange behavior related to this, though the listeners were getting removed. Previously, there was a comment noting that listeners could not be removed. Removal now works by making the listeners intance variables.

}
};

// We have to listen for both events: error means the process could not be launched and in that
// case exit will not be invoked.
this.process.on('error', onExit.bind((this)));
this.process.on('exit', onExit.bind((this)));
this.process.stdout.on('data', onStdoutData.bind(this));
this.process.stderr.on('data', onStdoutData.bind(this));

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

Why are we listening on stderr and stdout? We control the binary, so we should be able to figure out which pipe is being used.

Listening on both creates some pretty serious potential confusion, e.g. outputs could be interleaved arbitrarily.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

The process is using stderr. Not sure why this is the case, since Go ultimately calls log.Printf, which should output to stdout. Changed the implementation to listen on stderr only.

start(isUdpEnabled: boolean) {
// ./tun2socks \
// -tunName "tap0901:outline-tap0:10.0.85.2:10.0.85.0:255.255.255.0" \
private suspendListener() {

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

If you need a persistent bound instance of a method, you can use private suspendListener = () => {

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Great suggestion, done.

class SsLocal extends ChildProcessHelper {
constructor(private readonly proxyPort: number) {
super(pathToEmbeddedBinary('shadowsocks-libev', 'ss-local'));
// Class to manage the lifecycle of tun2socks. Silently restarts the process when

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

I don't see anything here that restarts the process when the system resumes. That's currently performed by ConnectionManager, on the network change event.

I do think it might be worth making this true, to avoid spreading the restart logic across these two classes.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

I updated the comment. See my comment below for an idea of how to resolve this.

@@ -316,56 +236,119 @@ class ChildProcessHelper {
this.process.kill();
}

set onExit(newListener: (() => void)|undefined) {
set onExit(newListener: (() => void) | undefined) {

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

I think this all might be clearer if the public exit listeners were replaced with Promises.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

Done.

console.log('system suspending');
// Windows: when the system suspends, tun2socks terminates due to the TAP device getting closed.
// Swap out the current listener, restart once the system resumes.
this.process.onExit = () => {

This comment has been minimized.

Copy link
@bemasc

bemasc Oct 24, 2019

Contributor

I think there's a race condition here, in this scenario:

  1. Tun2socks.suspendListener() runs due to a suspend event and overwrites this.process.onExit
  2. ConnectionManager.stop() -> Tun2socks.stop() -> ChildProcessHelper.stop() runs due to a user action
  3. Tun2socks.resumeListener() runs

In this case, ConnectionManager.onceStopped will never resolve, because it missed the event.

It's possible that Node provides some guarantee about the timing of suspend and resume events, but I would be surprised. Usually those kinds of events are pretty sloppy, occurring a few seconds before suspend and a few seconds after resume.

There might be a more elegant solution if Tun2socks handles the network change events itself, but I'm not entirely sure.

This comment has been minimized.

Copy link
@alalamav

alalamav Oct 24, 2019

Author Contributor

While the race is certainly possible, it seems hard to get into that state as I was not able to reproduce it. If it's an actual concern, I think a larger refactor/redesign would be necessary.

There might be a more elegant solution if Tun2socks handles the network change events itself, but I'm not entirely sure.

I'm not sure either. Tun2socks would have to know about the routing service, which is not ideal. I've been thinking a solution would be to expose an option in the binary to only perform the connectivity checks. This way we wouldn't have to restart tun2socks on every network check and we could contain (the TAP-processing) tun2socks lifecycle in Tun2socks. Wdyt?

@alalamav alalamav force-pushed the alalama-electron-go-ss branch from dca6414 to 8162d93 Oct 25, 2019
@alalamav alalamav marked this pull request as ready for review Nov 1, 2019
});
private stdErrListener?: (data?: string | Buffer) => void;

constructor(private path: string) { }

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

I think it would be more elegant to move launch into the constructor, kind of like RAII. That simplifies the API and enforces the requirement that launch can only be called once.

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Done.

// Use #onExit to be notified when the process exits.
stop() {
if (!this.process) {
// Never started.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

Or stop has already been called.

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Done.


// Returns a Promise that resolves if the connectivity checks succeed, indicating whehter
// UDP is supported. Rejects if the connectivity checks fail.
public get onceResult(): Promise<boolean> {

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

It looks like this is a boolean indicating UDP support, so I think the name should mention UDP.

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

I changed the method to return the process exit code and moved the UDP boolean logic to Tun2socks.

// UDP is supported. Rejects if the connectivity checks fail.
public get onceResult(): Promise<boolean> {
// NOTE: must return a Promise because getters cannot be async.
return new Promise(async (resolve, reject) => {

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

I may be getting old-fashioned here, but I wonder if this would be clearer as return this.tun2socks.onExit.then((code) => {

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

We're not chaining promises anymore, but I like it better too.

// Simple child process launcher.
//
// NOTE: Because there is no way in Node.js to tell whether a process launched successfully,
// #startInternal always succeeds; use #onExit to be notified when the process has exited

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

It looks like startInternal has been renamed?

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Done. Updated the documentation to reflect the new behavior.


constructor(private path: string) {}
private notifyStop = true;

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

I'm getting a little confused trying to understand this notifyStop thing, and I worry that it is error-prone. For example, if notifyStop becomes false (i.e. the process is restarted), and then the child process crashes without printing anything, Tun2socks.stopped will never resolve or reject.

Instead of having notifyStopped here, I think a better solution might be to makeChildProcessHelper.stop replace the exit listener (and be async to report successful shutdown), and have ChildProcessHelper.onExit only resolve on spontaneous exits.

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Good suggestion. Done.

console.log('system suspending');
// Windows: when the system suspends, tun2socks terminates due to the TAP device getting closed.
// Don't notify this exit.
this.notifyStop = false;

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

Is there any reason to wait for tun2socks to terminate itself, rather than terminating it ourselves here?

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Previously, we weren't calling stop to avoid unregistering the suspend/resume listeners. We're now stopping the process (directly) on suspend.

if (this.isUdpEnabled === await this.checkConnectivity()) {
return;
}
this.isUdpEnabled = !this.isUdpEnabled;

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 5, 2019

Contributor

This is clever, but I think it would be better to store await this.checkConnectivity() in a temp variable.

As an example of how this could go wrong, consider starting with isUdpEnabled = true. Two networkChanged events arrive in rapid succession, triggering two calls to checkConnectivity. If the second call resolves first, with value false, this code will flip isUdpEnabled to false. When the first call resolves later, also with value false, this code will flip isUdpEnabled again, to true!

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 6, 2019

Author Contributor

Done. We now update isUdpEnabled in checkConnectivity, caching the previous result as per your suggestion.

@bemasc
bemasc approved these changes Nov 6, 2019
Copy link
Contributor

bemasc left a comment

Thanks for making all these changes!

}
this.process.removeAllListeners();
const exit = new Promise(resolve => {
this.process!.once('exit', (code) => {

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Contributor

Technically this could be a race condition, depending on the precise concurrency behavior of node events. The previous line removed all listeners, so a spontaneous exit event could be lost before the new listener is added.

This is sufficiently unlikely that I don't think it's really necessary to fix, but it might be worth noting. (If you did want to fix it, you might be able to use this.process.killed to change the behavior of the exit handler...)

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 8, 2019

Author Contributor

I agree with you. I played around with retaining the original exit listener and only resolving the exit promise if !process.killed, but that opens up a bigger race condition given that we call process.removeAllListeners on the original exit listener, potentially removing the stop exit listener before it executes.

To minimize the risk of a race, I am now modifying the listeners and killing the process inside the stop() promise.

resolve(code);
});
});
this.process.kill();

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Contributor

The docs says this can throw an error (if the process has already shut down), so you might want to keep that in mind. It seems like an unlikely situation, but you might want to think about what would happen in that case (or just wrap kill() in a try-catch).

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 8, 2019

Author Contributor

Good point - I'm now checking for !this.running || this.process.killed in stop as an early return condition. In the docs, I didn't see that kill() can throw, but emits an 'error' event.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 11, 2019

Contributor

You're right, it emits 'error'. It looks like this code would lose that event, leading to an unresolved Promise. Could you add an 'error' listener here?

// case exit will not be invoked.
this.process.on('error', onExit.bind((this)));
this.process.on('exit', onExit.bind((this)));
if (!!this.process && this.process.isRunning) {

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Contributor

Can this be this.process?.isRunning?

This comment has been minimized.

Copy link
@alalamav

alalamav Nov 8, 2019

Author Contributor

Seems like this feature is only available starting typescript v3.7.2 and we're on v3.6.4 (on master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.