Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Ensure App.visit resolves when rendering completed. #16087

Merged
merged 2 commits into from
Jan 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"@glimmer/node": "^0.25.6",
"@glimmer/reference": "^0.25.6",
"@glimmer/runtime": "^0.25.6",
"@types/rsvp": "^4.0.1",
"aws-sdk": "^2.46.0",
"babel-plugin-check-es2015-constants": "^6.22.0",
"babel-plugin-debug-macros": "^0.1.10",
Expand Down
14 changes: 4 additions & 10 deletions packages/ember-application/lib/system/application-instance.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
*/

import { assign } from 'ember-utils';
import { get, set, run, computed } from 'ember-metal';
import { RSVP } from 'ember-runtime';
import { get, set, computed } from 'ember-metal';
import { environment } from 'ember-environment';
import { jQuery } from 'ember-views';
import EngineInstance from './engine-instance';
import { renderSettled } from 'ember-glimmer';

/**
The `ApplicationInstance` encapsulates all of the stateful aspects of a
Expand Down Expand Up @@ -243,14 +243,8 @@ const ApplicationInstance = EngineInstance.extend({
// No rendering is needed, and routing has completed, simply return.
return this;
} else {
return new RSVP.Promise((resolve) => {
// Resolve once rendering is completed. `router.handleURL` returns the transition (as a thennable)
// which resolves once the transition is completed, but the transition completion only queues up
// a scheduled revalidation (into the `render` queue) in the Renderer.
//
// This uses `run.schedule('afterRender', ....)` to resolve after that rendering has completed.
run.schedule('afterRender', null, resolve, this);
});
// Ensure that the visit promise resolves when all rendering has completed
return renderSettled().then(() => this);
Copy link
Contributor

@snewcomer snewcomer Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue Sorry to bring up something old, but seeing some memory leaks in fastboot related to retaining the Container. Is it necessary to return this in the callback? I think it is, but wondering if it is possible this never resolves but the app still is rendered.

What I'm worries about is never reaching a settled state or resolving, thus every request that comes in builds a new instance (w/ shoebox data) and the eventually swamps the server's heap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 3.5 right now

Copy link
Contributor

@snewcomer snewcomer Apr 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2019-04-12 at 13 58 39

Screenshot 2019-04-12 at 15 25 25
Screenshot 2019-04-12 at 15 12 49

some more info. I'm wondering if we need to "settle" the deferred promise when shouldRender is false....So not this block but the above block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really follow. Can you file an issue with a reproduction?

Copy link
Contributor

@snewcomer snewcomer Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue - We found it. Well @Serabe found it so I'll let him explain beyond this short answer! In an adapter, he found that headers as a computed was potentially leaking the container thus not allowing V8 to not gc anything. The problem wasn't necessarily ajax/najax/node-fetch but the way we configured our adapter.

// adapter
headers: computed(function() {

to a getter fixed it.

// adapter
get headers() {

We are on 3.5 and still investigating

}
};

Expand Down
1 change: 1 addition & 0 deletions packages/ember-glimmer/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ export {
InertRenderer,
InteractiveRenderer,
_resetRenderers,
renderSettled,
} from './renderer';
export {
getTemplate,
Expand Down
35 changes: 35 additions & 0 deletions packages/ember-glimmer/lib/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { RootReference } from './utils/references';
import OutletView, { OutletState, RootOutletStateReference } from './views/outlet';

import { ComponentDefinition, NULL_REFERENCE, RenderResult } from '@glimmer/runtime';
import RSVP from 'rsvp';

const { backburner } = run;

Expand Down Expand Up @@ -181,6 +182,39 @@ function loopBegin(): void {

function K() { /* noop */ }

let renderSettledDeferred: RSVP.Deferred<void> | null = null;
/*
Returns a promise which will resolve when rendering has settled. Settled in
this context is defined as when all of the tags in use are "current" (e.g.
`renderers.every(r => r._isValid())`). When this is checked at the _end_ of
the run loop, this essentially guarantees that all rendering is completed.

@method renderSettled
@returns {Promise<void>} a promise which fulfills when rendering has settled
*/
export function renderSettled() {
if (renderSettledDeferred === null) {
renderSettledDeferred = RSVP.defer();
// if there is no current runloop, the promise created above will not have
// a chance to resolve (because its resolved in backburner's "end" event)
if (!run.currentRunLoop) {
// ensure a runloop has been kicked off
backburner.schedule('actions', null, K);
}
}

return renderSettledDeferred.promise;
}

function resolveRenderPromise() {
if (renderSettledDeferred !== null) {
let resolve = renderSettledDeferred.resolve;
renderSettledDeferred = null;

backburner.join(null, resolve);
}
}

let loops = 0;
function loopEnd() {
for (let i = 0; i < renderers.length; i++) {
Expand All @@ -196,6 +230,7 @@ function loopEnd() {
}
}
loops = 0;
resolveRenderPromise();
}

backburner.on('begin', loopBegin);
Expand Down
73 changes: 73 additions & 0 deletions packages/ember-glimmer/tests/integration/render-settled-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import {
RenderingTestCase,
moduleFor,
strip
} from 'internal-test-helpers';
import { renderSettled } from 'ember-glimmer';
import { all } from 'rsvp';
import { run } from 'ember-metal';

moduleFor('renderSettled', class extends RenderingTestCase {
['@test resolves when no rendering is happening'](assert) {
return renderSettled().then(() => {
assert.ok(true, 'resolved even without rendering');
});
}

['@test resolves renderers exist but no runloops are triggered'](assert) {
this.render(strip`{{foo}}`, { foo: 'bar' });

return renderSettled().then(() => {
assert.ok(true, 'resolved even without runloops');
});
}

['@test does not create extraneous promises'](assert) {
let first = renderSettled();
let second = renderSettled();

assert.strictEqual(first, second);

return all([first, second]);
}

['@test resolves when rendering has completed (after property update)']() {
this.render(strip`{{foo}}`, { foo: 'bar' });

this.assertText('bar');
this.component.set('foo', 'baz');
this.assertText('bar');

return renderSettled().then(() => {
this.assertText('baz');
});
}

['@test resolves in run loop when renderer has settled'](assert) {
assert.expect(3);

this.render(strip`{{foo}}`, { foo: 'bar' });

this.assertText('bar');
let promise;

return run(() => {
run.schedule('actions', null, () => {
this.component.set('foo', 'set in actions');

promise = renderSettled().then(() => {
this.assertText('set in afterRender');
});

run.schedule('afterRender', null, () => {
this.component.set('foo', 'set in afterRender');
});
});

// still not updated here
this.assertText('bar');

return promise;
});
}
});
4 changes: 3 additions & 1 deletion packages/ember-metal/lib/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ interface IBackburner {
join(...args: any[]): void;
on(...args: any[]): void;
scheduleOnce(...args: any[]): void;
schedule(queueName: string, target: Object | null, method: Function | string): void;
}
interface IRun {
(...args: any[]): any;
schedule(...args: any[]): void;
later(...args: any[]): void;
join(...args: any[]): void;
backburner: IBackburner
backburner: IBackburner;
currentRunLoop: boolean;
}

export const run: IRun;
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@
"@types/acorn" "*"
"@types/source-map" "*"

"@types/rsvp@^4.0.1":
version "4.0.1"
resolved "https://registry.yarnpkg.com/@types/rsvp/-/rsvp-4.0.1.tgz#82956bc8d0a8151ec3b7e9cae64fd06808a1c714"

"@types/source-map@*":
version "0.5.2"
resolved "https://registry.yarnpkg.com/@types/source-map/-/source-map-0.5.2.tgz#973459e744cc7445c85b97f770275b479b138e08"
Expand Down