From da1a5616eb03dddb7cb31d4c7e33f1b8dd552d62 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 1 May 2018 14:04:13 +0100 Subject: [PATCH 1/3] Prevent error responses wedging group request concurrency limit Fixes https://github.com/vector-im/riot-web/issues/6592 --- src/stores/GroupStore.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/stores/GroupStore.js b/src/stores/GroupStore.js index 23ce5314ec..f4a66f432f 100644 --- a/src/stores/GroupStore.js +++ b/src/stores/GroupStore.js @@ -62,6 +62,11 @@ function limitConcurrency(fn) { } }) .then(fn) + .catch((err) => { + ongoingRequestCount--; + checkBacklog(); + throw err; + }) .then((result) => { ongoingRequestCount--; checkBacklog(); From 71c1198d12b5028147f69e6117f592e28afbaf36 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 1 May 2018 18:01:25 +0100 Subject: [PATCH 2/3] Rewrite limitConcurrency to fix error catching Make sure that we only catch errors that are a result of calling fn() --- src/stores/GroupStore.js | 44 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/stores/GroupStore.js b/src/stores/GroupStore.js index f4a66f432f..0e2e8e6b86 100644 --- a/src/stores/GroupStore.js +++ b/src/stores/GroupStore.js @@ -48,30 +48,30 @@ function checkBacklog() { // Limit the maximum number of ongoing promises returned by fn to LIMIT and // use a FIFO queue to handle the backlog. -function limitConcurrency(fn) { - return new Promise((resolve, reject) => { - const item = () => { - ongoingRequestCount++; - resolve(); - }; - if (ongoingRequestCount >= LIMIT) { - // Enqueue this request for later execution - backlogQueue.push(item); - } else { - item(); - } - }) - .then(fn) - .catch((err) => { +async function limitConcurrency(fn) { + if (ongoingRequestCount >= LIMIT) { + // Enqueue this request for later execution + await new Promise((resolve, reject) => { + backlogQueue.push(resolve); + }); + } + + let result; + let error; + + ongoingRequestCount++; + try { + result = await fn(); + } catch (err) { + error = err; + } finally { ongoingRequestCount--; checkBacklog(); - throw err; - }) - .then((result) => { - ongoingRequestCount--; - checkBacklog(); - return result; - }); + } + + if (error) throw error; + + return result; } /** From 2dfb3146b00f8d5264dc72b416754c6e7966c3fe Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 2 May 2018 10:39:15 +0100 Subject: [PATCH 3/3] Simplify concurrent request error handling --- src/stores/GroupStore.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/stores/GroupStore.js b/src/stores/GroupStore.js index 0e2e8e6b86..e7cea5667e 100644 --- a/src/stores/GroupStore.js +++ b/src/stores/GroupStore.js @@ -56,22 +56,16 @@ async function limitConcurrency(fn) { }); } - let result; - let error; - ongoingRequestCount++; try { - result = await fn(); + return await fn(); } catch (err) { - error = err; + // We explicitly do not handle the error here, but let it propogate. + throw err; } finally { ongoingRequestCount--; checkBacklog(); } - - if (error) throw error; - - return result; } /**