4 ms·
It's pretty easy to footgun when you're mixing event/callback based flow with promises/async. It's really easy to forget a closure in an event driven function
by claytongulick 3y ago
It's pretty easy to footgun when you're mixing event/callback based flow with promises/async.
It's really easy to forget a closure in an event driven function and to produce difficult to debug, error prone code.
A classic example I just screwed up:
export function getSFTPConnection(options) {
let key = readFileSync(options.keyfile_path, 'utf8');
let ssh = new ssh2.Client();
return new Promise(
(resolve, reject) => {
ssh.on('error', reject);
ssh.on('ready', () => {
ssh.sftp((err, sftp) => {
resolve(sftp);
});
});
ssh.on('close', (err) => {
reject(new Error('SSH connection closed: ' + err));
})
ssh.connect(
{
host: options.host,
username: options.username,
privateKey: key,
}
);
}
)
}
The bug here isn't obvious, at least to me - but it caused me much heartache and pain because I was moving too fast and not thinking - but a good example of unexpected behavior by default.
The bug is that there's not a closure around the ssh instance, so if you call this function multiple times it'll actually return the same connection instance.
The extra fun part of this bug is that the code worked - but because I was using the same instance, when I'd download multiple files in parallel, it would interleave the data, corrupting the files.
And the thing is, I know better. I've been doing js for decades.
I love js, but stuff like this is pretty frustrating, and can be a nightmare for new devs.
- minitech 3y ago> The bug is that there's not a closure around the ssh instance, so if you call this function multiple times it'll actually return the same connection instance. I’m not entirely sure what you mean by this, but it doesn’t sound like a correct diagnosis. Each call to `getSFTPConnection` creates a new `ssh2.Client` instance (unless `ssh2.Client`’s constructor does something really weird), and the promise can only resolve with the value `ssh.sftp` passes. (The error handling does look broken, though – I would expect the 'error' event to be able to fire at any time, and the `ssh.sftp` callback is missing a check.)
- claytongulick 3y agoYes, you're 100% correct, I mixed some stuff up when I was posting the code for simplicity. Bad example!
- plorkyeran 3y ago> The bug is that there's not a closure around the ssh instance, so if you call this function multiple times it'll actually return the same connection instance. You've either adjusted the code for posting or you misunderstood what the problem was, because no, that's not the case. The ssh variable is local to the getSFTPConnection function and is not reused between multiple calls to the function, and throwing a pointless extra closure in there wouldn't do anything.