Skip to content
This repository was archived by the owner on May 31, 2022. It is now read-only.

fix: Wait for the tunnel to close before finishing client close COMPASS-4474 #352

Merged
merged 9 commits into from
Mar 4, 2021
Merged

fix: Wait for the tunnel to close before finishing client close COMPASS-4474 #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
51f0a80
fix: Wait for the tunnel to close before finishing client close
gribnoysup Feb 24, 2021
e4c4576
fix: Update ssh-tunnel mock to match new implementation
gribnoysup Feb 24, 2021
61b0283
fix: Only pick up errors that originated in ssh-client
gribnoysup Feb 26, 2021
5beed8c
fix: Increase the timeout to make sure tunnel has a chance to start (…
gribnoysup Feb 26, 2021
7e1ac00
refactor: Async-ify create tunnel and connect tasks; Expose tunnel th…
gribnoysup Feb 26, 2021
851db7e
refactor: Pass tunnel as function parameter
gribnoysup Mar 2, 2021
97ebcd3
fix: Bump ssh-tunnel version once more
gribnoysup Mar 4, 2021
6158306
refactor: Asyncify awaiting on tunnel error
gribnoysup Mar 4, 2021
6a4c2b9
fix: Change spied close method
gribnoysup Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"editor.formatOnSave": true,
"editor.formatOnSave": false,
"editor.tabSize": 2,
"editor.insertSpaces": true,
"files.trimTrailingWhitespace": true,
Expand Down
74 changes: 46 additions & 28 deletions lib/connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { EventEmitter } = require('events');
const { EventEmitter, once } = require('events');
const fs = require('fs');
const async = require('async');
const {
Expand All @@ -12,7 +12,7 @@ const {
const { MongoClient } = require('mongodb');
const { parseConnectionString } = require('mongodb/lib/core');
const Connection = require('./extended-model');
const createSSHTunnel = require('./ssh-tunnel');
const { default: SSHTunnel } = require('@mongodb-js/ssh-tunnel');
Copy link
Contributor

Choose a reason for hiding this comment

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

😍


const debug = require('debug')('mongodb-connection-model:connect');

Expand Down Expand Up @@ -125,8 +125,9 @@ const getTasks = (model, setupListeners) => {
const tasks = {};
const _statuses = {};
let options = {};
let tunnel;
let client;
/** @type {SSHTunnel} */
let tunnel = null;
let client = null;

const status = (message, cb) => {
if (_statuses[message]) {
Expand Down Expand Up @@ -189,19 +190,27 @@ const getTasks = (model, setupListeners) => {
});

assign(tasks, {
[Tasks.CreateSSHTunnel]: (cb) => {
const ctx = status('Create SSH Tunnel', cb);
[Tasks.CreateSSHTunnel]: async() => {
const ctx = status('Create SSH Tunnel');

if (model.sshTunnel === 'NONE') {
return ctx.skip('The selected SSH Tunnel mode is NONE.');
}

tunnel = createSSHTunnel(model, ctx);
tunnel = new SSHTunnel(model.sshTunnelOptions);

try {
await tunnel.listen();
ctx(null);
} catch (err) {
ctx(err);
throw err;
}
}
});

assign(tasks, {
[Tasks.ConnectToMongoDB]: (cb) => {
[Tasks.ConnectToMongoDB]: async() => {
const ctx = status('Connect to MongoDB');

// @note: Durran:
Expand All @@ -215,6 +224,7 @@ const getTasks = (model, setupListeners) => {

validOptions.useNewUrlParser = true;
validOptions.useUnifiedTopology = true;

if (
model.directConnection === undefined &&
model.hosts.length === 1 &&
Expand All @@ -234,29 +244,37 @@ const getTasks = (model, setupListeners) => {
setupListeners(mongoClient);
}

mongoClient.connect((err, _client) => {
ctx(err);

if (err) {
if (tunnel) {
debug('data-service connection error, shutting down ssh tunnel');
tunnel.close();
/** @type {Promise<never>} */
const waitForTunnelError = (async() => {
const [error] = await once(tunnel || new EventEmitter(), 'error');
throw error;
})();

const closeTunnelOnError = async(tunnelToClose) => {
if (tunnelToClose) {
debug('data-service connection error, shutting down ssh tunnel');
try {
await tunnelToClose.close();
debug('ssh tunnel stopped');
} catch (err) {
debug('ssh tunnel stopped with error: %s', err.message);
}

return cb(err);
}
};

try {
const _client = await Promise.race([
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this: const [_client]? is there a test that verifies if we return a client and a tunnel?

Copy link
Contributor Author

@gribnoysup gribnoysup Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Promise.race returns only one value (it races a bunch of them against each other and only one "wins", right?), so it's not an array.

We have a whole suite of tests for connect method and they are still passing, so client is returned correctly, this implementation doesn't change current behavior. I haven't added any tests for returning tunnel because I initially thought that this will require setting up a proper tunnel server to test the whole thing and it seemed like too much for a trivial test like that, but I can mock a bunch of things and write one for returning tunnel, yeah

Copy link
Contributor

@mcasimir mcasimir Mar 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nice! makes a lot of sense :), could have checked myself.

mongoClient.connect(),
waitForTunnelError
]);
Comment on lines +266 to +269
Copy link
Contributor Author

@gribnoysup gribnoysup Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not really happy with this bit here as we are not really waiting for MongoClient.connect to resolve if tunnel threw (this should not affect anything though, so I think it's okay as is), but that's more or less the only way to do it without doing weird callback stuff as it was in the previous implementation

Copy link
Contributor

@mcasimir mcasimir Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good point!

Are user actionable errors about the tunnel thrown by .listen?

The one thing I was assuming here is this: all the ssh related errors are thrown on tunnel.listen ie if a cert or password is invalid .... and the only issues catched by on('error') are not caused by user mistakes (ie. they picked a wrong file) but by programming bug or network conditions that are not actionable beyond a retry: exception when the tcp connection is opened by MongoClient.

If that is true i think we can even ignore the race with waitForTunnelError (just use it to make sure tunnel closes), since MongoClient will throw a connection error and we could just give a nice error in the UI: The tunnel was open correctly but the connection to mongodb failed: ${mongoError}.

For sure we could give more info but is super fine, we are making it connect when it should, which now does not, we already improve the user life immensely.

If however instead errors like wrong password can only be catched by waitForTunnelError than is a different story and probably we should wait for that as you did, or better make sure those errors are thrown by listen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all the ssh related errors are thrown on tunnel.listen ie if a cert or password is invalid

This doesn't happen until first connection happens, tunnel is established for every incoming connection to ssh tunnel server, so things like forward out are just not happening before that. If we want them to be thrown when listen is called, some kind of ping request needs to be issued, which is doable. What do you think, should we add something like this here?

So in current implementation both errors emitted by tunnel or provided in callback can be helpful. But as mentioned, if tunnel emitted one, it probably will have a higher priority in terms of being useful to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

forward out are just not happening before that

When it gets to forward the connection to ssh should already be established, isn't it?

tunnel is established for every incoming connection to ssh

why? can't we just both create a new tunnel and 'forwarding tcp server' on listen instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why? can't we just both create a new tunnel and 'forwarding tcp server' on listen instead?

ssh2 docs don't recommend reusing one client for multiple connections, so @mongodb-js/ssh-tunnel creates new client for every incoming connection, which also means that client is not created until incoming connection is received

client = _client;

if (tunnel) {
client.on('close', () => {
debug('data-service disconnected. shutting down ssh tunnel');
tunnel.close();
});
}

cb(null, { url: model.driverUrlWithSsh, options: validOptions });
});
ctx(null);
return { url: model.driverUrlWithSsh, options: validOptions };
} catch (err) {
await closeTunnelOnError(tunnel);
ctx(err);
throw err;
}
}
});

Expand Down Expand Up @@ -332,7 +350,7 @@ const connect = (model, setupListeners, done) => {

logTaskStatus('Successfully connected');

return done(null, tasks.client, connectionOptions);
return done(null, tasks.client, tasks.tunnel, connectionOptions);
});

return tasks.state;
Expand Down
219 changes: 0 additions & 219 deletions lib/ssh-tunnel.js
View file
Open in desktop

This file was deleted.

8 changes: 8 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@
"scripts": {
"ci": "npm run check && npm test",
"pretest": "mongodb-runner install",
"test": "mocha",
"test": "mocha --timeout 15000",
"check": "mongodb-js-precommit"
},
"peerDependencies": {
"mongodb": "3.x"
},
"dependencies": {
"@mongodb-js/ssh-tunnel": "^1.2.0",
"ampersand-model": "^8.0.0",
"ampersand-rest-collection": "^6.0.0",
"async": "^3.1.0",
Expand Down
Loading