[FEAT] TrackableRequests for when async leakage is detected #5545
Conversation
runspired
force-pushed
the
add-traces-to-leaks
branch
from
ee5939d
to
acbe677
Compare
runspired
force-pushed
the
add-traces-to-leaks
branch
from
0a52592
to
1f7512e
Compare
|
|
||
| if (index !== -1) { | ||
| this._trackedAsyncRequests.splice(index, 1); | ||
| } |
There was a problem hiding this comment.
If their is no index, something seems wrong. We should explore/consider the impact of throwing.
| tracked.map(o => o.label).join('\n\t - ') | ||
| ); | ||
| } else { | ||
| warn( |
There was a problem hiding this comment.
by default, we may want to warn here. That way apps who violate this, have some grace period to cut over.
Erroring hard, make simply make it hard to upgrade with little upside. That being said, in the future we should strongly consider this being on by default.
| // debugging. Ideally we would have a tracked queue for requests with | ||
| // labels or local IDs that could be used to merge this trace with | ||
| // the trace made available when we detect an async leak | ||
| pendingFetchItem.trace = trace; |
There was a problem hiding this comment.
eventually we should thread this through, and it super supersede the _trackAsyncRequestStart trace
tests/unit/store/async-leak-test.js
Outdated
|
|
||
| test('the waiter properly waits for pending requests', async function(assert) { | ||
| let stepResolve; | ||
| let stepPromise = new Promise(resolveStep => { |
tests/unit/store/async-leak-test.js
Outdated
|
|
||
| assert.equal(waiter(), false, 'We return false to keep waiting while requests are pending'); | ||
|
|
||
| await request.catch(e => { |
There was a problem hiding this comment.
assert.rejects if present in this version of QUnit, could be used to provide better test failure messages.
more context: https://github.com/qunitjs/qunit/pull/1238/files#diff-78b865bceaf70054fdd9917848e7ddd8R43
runspired
force-pushed
the
add-traces-to-leaks
branch
from
1f7512e
to
6401c44
Compare
improve waiter erroring improve trace infra and add tests only run waiter tests in dev
runspired
force-pushed
the
add-traces-to-leaks
branch
from
06ca335
to
bc1bdac
Compare
This feature enables app developers to better use
async ... awaitwhile simultaneously detecting asynchronous test leaks in their data layer. It is best paired with ember-qunit'ssetupTestIsolationValidationfeature.When running an
ember-cliapplication in non-production environments, this feature adds a test-waiter per-store instance that works with thesettled()test-helper. This test-waiter does not cause waiting by default, in order to prevent immediately breaking any apps that upgrade to this version of ember-data that do have test leaks without warning.This feature comes with two feature flags, only configurable in
non-productionenvironments, that live on thestoreinstance. Both of these flags are currentlyfalseby default.store.shouldTrackAsyncRequestsWhen
true, requests initiated via the store will be assigned a token, and while any tokens have not resolved the test-waiter (and thusawait settled()) will wait.When
true, any detected leaks will throw an error when the store is destroyed after a test, causing the test to fail. The leaks will be logged, with a descriptivelabelfor the request that was initiated, and potentially atrace(see below) for where the request initiated from.When
false, any detected leaks will be emitted as warnings when the store is destroyed after a test.store.generateStackTracesForTrackedRequestsWhen
true, a stack trace will be captured at the point that a request is initiated and saved onto the token used to track the request. For requests other thanfindRecord, these traces will include the point inappcode that caused this request to be initiated. ForfindRecordthese traces currently point back toflushPendingFetches; however, we also capture a trace at the point that we schedule these fetches, and will soon thread those traces through so that where the request initiated inappcode is clear.