-
Notifications
You must be signed in to change notification settings - Fork 236
fix(ssh-tunnel): reconnect the ssh tunnel when it gets disconnected COMPASS-5454 #3096
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
Conversation
b330869
to
c750e78
Compare
|
||
this.sshClient = new SshClient(); | ||
|
||
this.sshClient.on('close', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the 'close' event will always be called when a socket is closed, whether a connection was ended gracefully or closed due to error. Speaking of which, you'll want to add an 'error' event handler for the connection in case of errors, to prevent the process from dying by default.
packages/ssh-tunnel/src/index.ts
Outdated
} | ||
|
||
debug('creating SSH connection'); | ||
const ac = new AbortController(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AbortController allows us to cancel listening to the 'error' and 'ready' events when the Promise.race resolves or rejects. A bit minor in the bigger scheme of things, but would technically be a leak?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! didn't know once
accepted a signal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s only supported in Node.js 15+, I think this might not be working yet? But I guess it’s fine to leave it like this and see it start working when we upgrade Electron :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory we have an abortcontroller polyfill, but yeah - probably doesn't touch events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine this way - the leaks were already there and they are super minor. You're only going to make so many ssh tunnels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just removed the AbortController stuff for now because it was spiralling out and out. It requires jsdom so I'd have to systematically move more and more packages onto our react mocha config so that the polyfill is there when it uses ssh-tunnel.
c750e78
to
4c929c1
Compare
4c929c1
to
825b6a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Only one comment on the tests :)
No description provided.