Skip to content

Expose option to allow a new sandbox per visit #252

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Expose option to allow a new sandbox per visit #252

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b953b5
Expose option to allow a new sandbox per visit
Nov 4, 2019
3b28a2e
Update documentation for `buildSandboxPerVisit` option.
Jan 31, 2020
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
37 changes: 30 additions & 7 deletions src/ember-app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,9 @@ class EmberApp {
* Perform any cleanup that is needed
*/
destroy() {
// TODO: expose as public api (through the top level) so that we can
// cleanup pre-warmed visits
if (this._applicationInstance) {
this._applicationInstance.destroy();
}
}

/**
Expand Down Expand Up @@ -242,12 +243,27 @@ class EmberApp {
* @param {Object} bootOptions An object containing the boot options that are used by
* by ember to decide whether it needs to do rendering or not.
* @param {Object} result
* @param {Boolean} buildSandboxPerVisit if true, a new sandbox will
* **always** be created, otherwise one
* is created for the first request
* only
* @return {Promise<instance>} instance
*/
async visitRoute(path, fastbootInfo, bootOptions, result) {
let app = await this.buildApp();
result.applicationInstance = app;

async _visit(path, fastbootInfo, bootOptions, result, buildSandboxPerVisit) {
let shouldBuildApp = buildSandboxPerVisit || this._applicationInstance === undefined;

let app = shouldBuildApp ? await this.buildApp() : this._applicationInstance;

if (buildSandboxPerVisit) {
// entangle the specific application instance to the result, so it can be
// destroyed when result._destroy() is called (after the visit is
// completed)
result.applicationInstance = app;
} else {
// save the created application instance so that we can clean it up when
// this instance of `src/ember-app.js` is destroyed (e.g. reload)
this._applicationInstance = app;
}
Comment on lines +255 to +266
Copy link
Contributor

@kratiahuja kratiahuja Nov 4, 2019

Choose a reason for hiding this comment

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

I think this is a bit confusing in terms of variable naming. IIUC, we are deciding here whether to build a new EmberApp every request or re-use the old one and still create a new EmberAppInstance. Please correct me if I am wrong.

If that is the case, shouldn't we call it this._application and result.application. It isn't an instance per say. I know the already existing code calls it appInstance and appInstanceInstance` but that can easily cause confusion :)

Copy link
Member Author

@rwjblue rwjblue Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a bit confusing in terms of variable naming.

Yeah, I don't disagree, but we are talking about instances of Ember.Application and Ember.ApplicationInstance. We have to call them something...

It isn't an instance per say.

Well, it is actually. It is the result of Ember.Application.create(), it is an Ember.Application instance (hence the name this._applicationInstance). The other thing we get is an Ember.ApplicationInstance instance (and is called result.applicationInstanceInstance).

tldr; I erred on the side of "correct but overly verbose", I could change it to .application and .instance if you prefer.

Copy link
Contributor

@kratiahuja kratiahuja Nov 5, 2019

Choose a reason for hiding this comment

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

yeah my only concern was around accidentally misread of the variables due to repeated subwords.

Copy link
Member Author

@rwjblue rwjblue Nov 6, 2019

Choose a reason for hiding this comment

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

Yeah :( including Instance in the name of a class is bad news bears 😭

Copy link

@dnalagatla dnalagatla Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

why not use app as variable name? I agree using appInstance as a variable name is a bit confusing while reading the code :(

Copy link
Member Author

@rwjblue rwjblue Nov 6, 2019

Choose a reason for hiding this comment

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

“App” is significantly overused as well. Is it an instance of src/ember-app.js in this project? Is it an instance of lib/broccoli/ember-app.js in ember-cli? Is it an Ember.Application instance? Is it the conceptual “application” that we are rendering? 🙀

Tbh, I’d rather make up completely fabricated names (that don’t use “instance” at all) and link to a naming doc to explain what each of the names mean.

Copy link

@dnalagatla dnalagatla Nov 6, 2019

Choose a reason for hiding this comment

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

I agree, it's a tough problem.

await app.boot();

let instance = await app.buildInstance();
Expand Down Expand Up @@ -278,6 +294,7 @@ class EmberApp {
* @param {Boolean} [options.shouldRender] whether the app should do rendering or not. If set to false, it puts the app in routing-only.
* @param {Boolean} [options.disableShoebox] whether we should send the API data in the shoebox. If set to false, it will not send the API data used for rendering the app on server side in the index.html.
* @param {Integer} [options.destroyAppInstanceInMs] whether to destroy the instance in the given number of ms. This is a failure mechanism to not wedge the Node process (See: https://github.com/ember-fastboot/fastboot/issues/90)
* @param {Boolean} [options.buildSandboxPerVisit] whether to create a new sandbox context per-visit, or reuse the existing sandbox
* @param {ClientRequest}
* @param {ClientResponse}
* @returns {Promise<Result>} result
Expand Down Expand Up @@ -314,7 +331,13 @@ class EmberApp {
}

try {
await this.visitRoute(path, fastbootInfo, bootOptions, result);
await this._visit(
path,
fastbootInfo,
bootOptions,
result,
options.buildSandboxPerVisit === true
);

if (!disableShoebox) {
// if shoebox is not disabled, then create the shoebox and send API data
Expand Down
1 change: 1 addition & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class FastBoot {
* @param {Boolean} [options.shouldRender] whether the app should do rendering or not. If set to false, it puts the app in routing-only.
* @param {Boolean} [options.disableShoebox] whether we should send the API data in the shoebox. If set to false, it will not send the API data used for rendering the app on server side in the index.html.
* @param {Integer} [options.destroyAppInstanceInMs] whether to destroy the instance in the given number of ms. This is a failure mechanism to not wedge the Node process (See: https://github.com/ember-fastboot/fastboot/issues/90)
* @param {Boolean} [options.buildSandboxPerVisit] whether to create a new sandbox context per-visit (slows down each visit, but guarantees no prototype leakages can occur), or reuse the existing sandbox (faster per-request, but each request shares the same set of prototypes)
* @returns {Promise<Result>} result
*/
async visit(path, options = {}) {
Expand Down
13 changes: 8 additions & 5 deletions test/fastboot-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,45 +401,48 @@ describe('FastBoot', function() {
});
});

it('in app prototype mutations do not leak across visits', async function() {
it('in app prototype mutations do not leak across visits with buildSandboxPerVisit=true', async function() {
this.timeout(3000);

var fastboot = new FastBoot({
distPath: fixture('app-with-prototype-mutations'),
});

let result = await fastboot.visit('/');
let result = await fastboot.visit('/', { buildSandboxPerVisit: true });
let html = await result.html();

expect(html).to.match(/Items: 1/);

result = await fastboot.visit('/');
result = await fastboot.visit('/', { buildSandboxPerVisit: true });
html = await result.html();

expect(html).to.match(/Items: 1/);

result = await fastboot.visit('/');
result = await fastboot.visit('/', { buildSandboxPerVisit: true });
html = await result.html();

expect(html).to.match(/Items: 1/);
});

it('errors can be properly attributed', async function() {
it('errors can be properly attributed with buildSandboxPerVisit=true', async function() {
this.timeout(3000);

var fastboot = new FastBoot({
distPath: fixture('onerror-per-visit'),
});

let first = fastboot.visit('/slow/100/reject', {
buildSandboxPerVisit: true,
request: { url: '/slow/100/reject', headers: {} },
});

let second = fastboot.visit('/slow/50/resolve', {
buildSandboxPerVisit: true,
request: { url: '/slow/50/resolve', headers: {} },
});

let third = fastboot.visit('/slow/25/resolve', {
buildSandboxPerVisit: true,
request: { url: '/slow/25/resolve', headers: {} },
});

Expand Down