fix(trace): survive sw restart #37442
Conversation
| "version": "0.0.0", | ||
| "type": "module", | ||
| "dependencies": { | ||
| "idb-keyval": "^6.2.2", |
There was a problem hiding this comment.
sw.bundle.js
before
220990
after
223446
Co-authored-by: Yury Semikhatsky <[email protected]> Signed-off-by: Pavel Feldman <[email protected]>
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 7 flaky46869 passed, 821 skipped Merge workflow run. |
cpAdm
left a comment
cpAdm
left a comment
There was a problem hiding this comment.
@pavelfeldman Thanks for the effort! Unfortunately, it is not quite fixed, due to the /ping calling gc before we reload the traces, see PR comment.
| const oldValue = await idbKeyval.get('clientIdToTraceUrls'); | ||
| if (newValue === oldValue) | ||
| return; | ||
| idbKeyval.set('clientIdToTraceUrls', newValue); |
There was a problem hiding this comment.
I think we want to await this
| loadedTraces.delete(traceUrl); | ||
| } | ||
|
|
||
| await saveClientIdParams(); |
There was a problem hiding this comment.
Saving here is troublesome:
When we re-open the browser tab, the service worker starts again, and the /pings requests come in, which will call gc.
At this point clientIdToTraceUrls is empty and thus we clear the database, even tough this client has traces.
So, I suggest to either only call saveClientIdParams if we call clientIdToTraceUrls.delete
and/or move the if (!clientIdToTraceUrls.has(event.clientId)) { check before the /ping handling.
| const params = await loadClientIdParams(event.clientId); | ||
| if (params) { | ||
| for (const traceUrl of params.traceUrls) | ||
| await loadTrace(traceUrl, null, client, params.limit, () => {}); |
There was a problem hiding this comment.
loadTrace also calls gc. Not really what we need here, but not sure if that can cause (concurrency) issues?
Maybe move gc out of loadTrace and just call it in the relativePath === '/contexts' branch?
| } | ||
| } | ||
|
|
||
| if (!clientIdToTraceUrls.has(event.clientId)) { |
There was a problem hiding this comment.
For the 'trace/snapshot/' client this will always be true.
| * limitations under the License. | ||
| */ | ||
|
|
||
| import * as idbKeyval from 'idb-keyval'; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| await saveClientIdParams(); | ||
| } | ||
|
|
||
| // Persist clientIdToTraceUrls to localStorage to avoid losing it when the service worker is restarted. |
There was a problem hiding this comment.
Technically IndexDB !== localStorage
This reverts commit 25f89ac.
3 participants
Fixes #37421