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

Enable FastBoot #1715

Merged
merged 15 commits into from Sep 5, 2019

Conversation

@kzys
Copy link
Contributor

commented Apr 11, 2019

Actually, without completely removing jQuery, FastBoot seems working.

@kzys kzys changed the title Enable fastboot Enable FastBoot Apr 11, 2019

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Hmm, actually curl 'http://localhost:4200/crates/nanomsg' -H 'Accept: text/html' didn't work on npm run start:live

@kzys kzys referenced this pull request Apr 11, 2019
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

☔️ The latest upstream changes (presumably #1725) made this pull request unmergeable. Please resolve the merge conflicts.

@kzys kzys force-pushed the kzys:enable-fastboot branch from 7c1931d to 7a519e0 Apr 25, 2019

@jtgeibel

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Will we need an additional process in front of the backend to intercept and build responses for non-API requests? I've deployed this to a staging area, but I don't see it working in curl either: curl -H 'Accept: text/html' https://jtgeibel-staging-crates-io.herokuapp.com/.

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Yep. Seems like so.

https://github.com/ember-fastboot/fastboot-app-server

How about merging this change first, and have a different PR to change our Heroku configuration?

There are multiple PRs that touch package.json. I'd like to avoid rebasing this branch again and again while we are figuring out Heroku deployment configuration.

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

☔️ The latest upstream changes (presumably #1733) made this pull request unmergeable. Please resolve the merge conflicts.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

We can't merge a branch that is undeployable.

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Let me double-check. My understanding is that we need an additional server to make server-side rendering work, but the front-end server itself is working as like before.

@jtgeibel

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I'm not sure if we will need to add fastboot-app-server or if we will just be able to spawn ember serve from the Rust backend and proxy the applicable requests to it. Unfortunately I haven't been able to get either configuration working locally.

For me, ember serve looks like it works (because the browser still dynamically loads the content), but the HTML it serves up doesn't actually populate any data, I just see comments where I assume fastboot would inject pre-rendered HTML. Do we need to explicitly list some dependencies as described here?

@locks locks self-assigned this Jun 20, 2019

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Hey. I'm back! Let me remove jQuery first before enabling FastBoot.

@kzys kzys force-pushed the kzys:enable-fastboot branch from 7a519e0 to 12bc4db Jul 11, 2019

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

I have revamped the PR. Here is my Heoku environment. https://fathomless-island-39956.herokuapp.com

@jtgeibel - I have added Fastboot Server since it is recommended by https://ember-fastboot.com/docs/deploying. Currently Nginx is still receiving requests from Heroku's router and forwarding certain requests (just "/" at this time) to the Fastboot Server.

@jtgeibel

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Great @kzys, this is looking a lot better! I tweaked your nginx conf a bit to try things out on more pages. (You can try it in my staging area, at: https://jtgeibel-staging-crates-io.herokuapp.com/) Here are some of the things I noticed:

  • The homepage is partially pre-rendered, but the data is still loaded via AJAX after the page loads. For example, a crate count of "---,---" is returned in the HTML. I believe this was done intentionally in the frontend so that this page loads quickly even if the summary endpoint is slow to respond. Recently some improvements have gone into the summary query so we can consider changing this. I don't see this is a blocker, just something we can consider changing so that this works correctly when JS is disabled.
  • /crates and /crates/test-crate appear to work fine, populating all of the expected data.
  • Visiting pages that require authentication, causes the page to redirect to /. For example, /me, /dashboard, and /me/pending-invites behave this way when you navigate to the URL directly. Once the Ember frontend has loaded (JS enabled), it is still possible to navigate to these URLs using the menu. What I expect is happening is that the user's cookie is sent to the fastboot server, but that this cookie is not used when the fastboot server makes requests to the backend. This then results in a redirect in checkCurrentUser.
  • /search?q=test-crate loads a page that says it is loading search results. I assume this was deferred on purpose at some point, similar to the index page mentioned above. (With JS enabled, the search does populate after sending the AJAX reqeust. Without JS, no results are shown.)
  • With JS disabled, /install simply says that it is redirecting the user to the documentation. We can probably return a redirect directly from the server.
  • The index / and /policies pages load and fill more of the page width than normal. Once ember loads, the width then shrinks down to its normal size. I haven't looked yet to see what is different in the HTML that is apparently selecting different CSS styling.

Overall, it seems like the main fastboot functionality is working, with just a few rough edges to resolve!

@jtgeibel

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

I also just noticed, that for /crates/test-crate and /crates/test-crate/0.1.5 the "Authors", and "Owners" sections are not populated.

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I have fixed the styling issue.

Due to the impact, I'd like to enable FastBoot on a few pages (maybe /install and /policies?) first to see to make sure things are working correctly. What do you think?

@jtgeibel

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

@kzys Sorry for the delay in getting back to you here.

I have fixed the styling issue.

It looks like that CSS in that patch is also being applied to the download graph, causing the width to overflow. I've sent you an invite on Percy so that you should have access to any visual diffs identified there.

Due to the impact, I'd like to enable FastBoot on a few pages (maybe /install and /policies?) first to see to make sure things are working correctly. What do you think?

I think this sounds like a great approach to minimize any potential ops impacts. I've been doing some experiments with your branch on Heroku, and I've organized the points below based on this sort of incremental rollout.

Potential blockers to rolling out rendering of static pages

  • I ran into a dependency issue when experimenting locally. I was getting an error when running node ./fastboot.js and I had to add fastboot-app-server to pacakge.json and then run npm install. I'm not sure how this was working in my Heroku staging area. I'm guessing that this dependency was included at one point and that it was still in the cache from a previous deploy.
  • I'm seeing a minimal impact to memory usage in staging, however I don't want to send a ton of traffic to Heroku from my IP, so this may not hold in production. This is definitely a good reason to roll out a few pages at a time as you've proposed.
  • The backend writes to /tmp/app-initialized after it has booted. This notifies the nginx buildpack that the dyno is ready to start accepting inbound traffic. Locally I'm seeing it take about 3 seconds for the fastboot server to start, so I think we will want to implement a similar scheme here. If the fastboot server would write to a similar temporary file, we could wait for this file before the backend signals that we're ready to accept client requests.

Potential blockers for rendering of dynamic pages

  • I noticed that for dynamic pages, after ember loads it will automatically send API requests to the backend, even though all the necessary data was already pre-rendered by the fastboot server. It seems like we would want to inhibit this batch of requests until the user navigates to a new frontend route. Otherwise this will double the number of requests hitting the backend when users first navigate to the site.
  • When navigating to a page that doesn't exist (such as a non-existent crate name), the fastboot server returns the expected "Not found" text, but in a status 200 response. It would be nice if we could have the server convert this into a 404 response. On the fastboot server, I'm seeing a type error along the lines of "couldn't read property 'get' of undefined".
@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Thanks. Let me address the blockers for static pages.

  • Missing fastboot-app-server: Ah, right. It should be there.
  • /tmp/app-initialized: Okay. Will do.
@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

The graph issue is a bit strange, because the graph div should be always under body>div. Do the acceptance tests have a different HTML structure?

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Okay. This is because of #ember-testing container that has ember-application class...

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Since package.json and package-lock.json tend to cause a lot of conflicts, it would be great if we can focus on a small set of low-traffic static pages first, merge this branch and fix other pages in different, hopefully smaller pull requests.

@kzys kzys force-pushed the kzys:enable-fastboot branch from 802d0f9 to 6a650b2 Aug 2, 2019

@locks locks referenced this pull request Aug 2, 2019
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

☔️ The latest upstream changes (presumably #1787) made this pull request unmergeable. Please resolve the merge conflicts.

@jtgeibel

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Since package.json and package-lock.json tend to cause a lot of conflicts, it would be great if we can focus on a small set of low-traffic static pages first, merge this branch and fix other pages in different, hopefully smaller pull requests.

I definitely agree with this path forward. I'm deploying to staging now to test out the latest changes, but they look good to me.

From my perspective, this PR will be good to merge with:

  • Fix new merge conflicts
  • Update nginx config to route some static pages from fastboot as you describe.

@sgrif do you have any concerns with deploying this to begin testing fastboot in production for a few static pages? I think this is a good way forward to try things out with the minimal potential for impacting production, but I'd like to make sure you are comfortable from an ops/deployment perspective before merging to master.

@kzys kzys force-pushed the kzys:enable-fastboot branch from 6a650b2 to cff7481 Aug 8, 2019

@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

ddfeed7 changes the FastBoot-enabled pages from / to only /policies. It might be really conservative, but I'd like to start small.

The other 2 commits are for getting rid of timestamps, since Heroku adds them anyway.

@jtgeibel
Copy link
Member

left a comment

CI had failed on a dpkg install, so I restarted it. Now its showing 3 lints in fastboot.js causing the build to fail. Other than that and a new review comment, this is looking great to me!

<noscript>
<div id="main">
<div class='noscript'>
This site requires JavaScript to be enabled.

This comment has been minimized.

Copy link
@jtgeibel

jtgeibel Aug 13, 2019

Member

Doing a final pass on these changes, I'm not sure if we should remove this notice at this time. We'll want to make sure users hitting URLs other than /policy see this. Is there a way to only remove this when rendering in fastboot?

This comment has been minimized.

Copy link
@kzys

kzys Aug 13, 2019

Author Contributor

Good catch! Let me restore the warning for now. index.html cannot use Handlerbars apparently. So checking FastBoot from here seems tricky.

@sgrif

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

do you have any concerns with deploying this to begin testing fastboot in production for a few static pages?

As long as it's some relatively low traffic pages I think that's fine

kzys added 2 commits Aug 13, 2019
@kzys

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

I can squash commits to either one or a few if you want to. Please let me know!

@jtgeibel
Copy link
Member

left a comment

Approving, but not merging at the moment as we'll probably want to deploy to production as an individual change.

@jtgeibel jtgeibel referenced this pull request Aug 20, 2019
1 of 18 tasks complete
@kzys kzys referenced this pull request Aug 30, 2019
@locks
locks approved these changes Sep 4, 2019
@sgrif

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@bors r=jtgeibel

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

📌 Commit 56f23f1 has been approved by jtgeibel

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

⌛️ Testing commit 56f23f1 with merge 111b7eb...

bors added a commit that referenced this pull request Sep 5, 2019
Auto merge of #1715 - kzys:enable-fastboot, r=jtgeibel
Enable FastBoot

Actually, without completely removing jQuery, FastBoot seems working.
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing 111b7eb to master...

@bors bors merged commit 56f23f1 into rust-lang:master Sep 5, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
percy/crates.io Visual review automatically approved, no visual changes found.
Details
sgrif added a commit to sgrif/crates.io that referenced this pull request Sep 5, 2019
Revert "Auto merge of rust-lang#1715 - kzys:enable-fastboot, r=jtgeibel"
This reverts commit 111b7eb, reversing
changes made to 27bd9a8.

This caused our memory usage to increase from 50MB to 350MB at boot, and
I was able to trivially cause the server to run out of memory by hitting
/policies repeatedly. This introduces a DOS vector that we can't afford.
bors added a commit that referenced this pull request Sep 5, 2019
Auto merge of #1827 - sgrif:sg-revert-1715, r=sgrif
Revert "Auto merge of #1715 - kzys:enable-fastboot, r=jtgeibel"

This reverts commit 111b7eb, reversing
changes made to 27bd9a8.

This caused our memory usage to increase from 50MB to 350MB at boot, and
I was able to trivially cause the server to run out of memory by hitting
/policies repeatedly. This introduces a DOS vector that we can't afford.
@sgrif

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

This caused memory issues, I've reverted in #1827

@kzys kzys referenced this pull request Sep 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.