Immediately destroy stream created in rejected Promise.all()

[matthew-white]

We saw in #482 that the following sequence is possible:

• A stream is created within a Promise.all().
• The stream is to be processed after the Promise.all() is fulfilled.
• However, another promise in the Promise.all() is rejected.
• As a result, the Promise.all() as a whole is rejected.
• As a result, the stream is never processed and isn't destroyed.

We fixed #482 by destroying any stream that hasn't been read from in 2 minutes (#604). That's a general fix that also addresses other problem scenarios. However, in the sequence above, it would be great if we could destroy the stream more quickly than 2 minutes. I think this comes up in three places, all involving submissions:

central-backend/lib/resources/submissions.js

Lines 270 to 277 in 851f30f

	return Promise.all([
	(options.deletedFields === true) ? Forms.getMergedFields(form.id) : Forms.getFields(form.def.id),
	Submissions.streamForExport(form.id, draft, keys, options),
	(options.splitSelectMultiples !== true) ? null : Submissions.getSelectMultipleValuesForExport(form.id, draft, options),
	SubmissionAttachments.streamForExport(form.id, draft, keys, options),
	ClientAudits.streamForExport(form.id, draft, keys, options),
	draft ? null : Audits.log(auth.actor, 'form.submission.export', form)
	]).then(([fields, rows, selectValues, attachments, clientAudits]) => {

central-backend/lib/resources/submissions.js

Lines 296 to 302 in 851f30f

	return Promise.all([
	(options.deletedFields === true) ? Forms.getMergedFields(form.id) : Forms.getFields(form.def.id),
	Submissions.streamForExport(form.id, draft, Object.keys(passphrases), options),
	(options.splitSelectMultiples !== true) ? null : Submissions.getSelectMultipleValuesForExport(form.id, draft, options),
	Keys.getDecryptor(passphrases),
	draft ? null : Audits.log(auth.actor, 'form.submission.export', form)
	])

central-backend/lib/resources/odata.js

Lines 66 to 71 in 851f30f

	return Promise.all([
	Forms.getFields(form.def.id).then(selectFields(query, params.table)),
	Submissions.streamForExport(form.id, draft, undefined, options),
	((params.table === 'Submissions') && options.hasPaging())
	? Submissions.countByFormId(form.id, draft, options) : resolve(null)
	])

I believe that #564 would fix the first two cases by moving the stream(s) out of the Promise.all(). We'd still have to fix the third case. #608 takes a similar approach to #564 and fixes the third case as well, but it didn't seem to perform well when we benchmarked it. We also have #609, which takes a different approach to fix the three cases.

We're planning to return to this issue after finishing our work with #564.

[matthew-white]

We've decided to close this issue. We know that there are other, more likely situations in which a stream won't be destroyed for 2 minutes. The main example that comes to mind is premature closes, which are pretty common and can lead to a stream lingering for 2 minutes. In contrast, the case described in this issue should be pretty uncommon. The main time that we've seen it is when there is already other pressure on the database connection pool, resulting in a Backend request acquiring a connection for the stream but not the next query after the stream. So if there is already pressure on the connection pool, this issue could exacerbate it. But we don't expect such pressure on the connection pool in normal usage: now that pm2 is working, Backend has access to up to 40 database connections (10 connections × 4 pm2 instances). Even if the connection pool does become full, Backend will destroy the stream after 2 minutes, reclaiming the connection. The main concern I have with this issue is that it could impede load testing, during which we expect there to be pressure on the connection pool. However, it doesn't seem like that's been the case in the load testing that we've done so far.