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

Delay building web.browser.legacy bundle until after development server restarts. #10055

Merged
merged 22 commits into from Jul 11, 2018

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Jul 5, 2018

Now that Meteor builds two client bundles, build times have naturally increased a bit. We are hard at work attacking this problem from multiple angles, such as #9983, and I'm confident that Meteor 1.7.1 build times will be shorter for most apps than they were before Meteor 1.7, despite building both web.browser and web.browser.legacy bundles.

Nevertheless, since most testing in development happens using one bundle/browser at a time, it has been suggested (#9948) that we could skip the legacy build in development, as a cheap short-cut to faster rebuild times. To be honest, I don't love most of these ideas for disabling the legacy build with some sort of additional configuration, because supporting older browsers is important, configuration is a creeping cancer, and ignoring errors in the legacy build seems like a recipe for frustration when it's time to deploy your app.

I would vastly prefer a zero-configuration solution to this problem, though that's challenging in this case because skipping an important part of the build process seems like a decision the developer should make consciously and carefully.

As this pull request demonstrates, it's possible to remove the legacy build from the critical path to restarting the development server, so you can continue testing and developing your app while the legacy bundle is built in the background. There's nothing to configure, because the legacy bundle still gets built in roughly the same amount of time as before; you just don't have to wait for it to finish, if you aren't currently interested in testing legacy code.

How it works in detail:

  • All bundles are built when the build tool first starts up, so that there is always a legacy bundle available, even if it's slightly stale, and commands like meteor build and meteor deploy still work as before.
  • On each subsequent rebuild of the application, we delay starting the web.browser.legacy build until after all other builds have finished and the development server has restarted.
  • If you access your application using a legacy browser before the legacy build has finished, you'll just get the previous (possibly slightly stale) legacy code. Not the end of the world!
  • As soon as the legacy build finishes, the build process sends a message to the webapp package running in the server process to tell it to reload the latest legacy bundle.
  • Any errors building the legacy bundle get reported in the same way as before.
  • Once everything has finished, you'll see the following message in the console:
    => Finished delayed build of web.browser.legacy in 951ms
    
    This message means it's now safe to test your app in a legacy browser, if that's something you were trying to do.

In short, when we talk about zero-configuration here at the Meteor project, we mean something much deeper than just picking sensible defaults. No, we are striving to eliminate the need for configuration whenever and wherever we can. If you want to hear more about this cornerstone of the Meteor philosophy, check out this Meteor Night talk that I recently gave about the Meteor 1.7 modern/legacy bundling system (or read the slides).

benjamn added 9 commits Jul 3, 2018
Most importantly, we no longer return a copy of previousBuilders from
bundler.bundle, but simply modify the input object over time. The copying
approach was nice in theory, but incompatible with delaying the bundling
of certain architectures (e.g. web.browser.legacy) until some time after
bundler.bundle returns.
@benjamn benjamn added this to the Release 1.7.1 milestone Jul 5, 2018
@benjamn benjamn self-assigned this Jul 5, 2018
@benjamn benjamn requested review from hwillson and abernix Jul 5, 2018
};
process.on("message", async (message) => {
if (message.package !== "webapp") return;
if (message.method === "generateClientProgram") {

This comment has been minimized.

@benjamn

benjamn Jul 5, 2018
Author Member

It's possible that the parent process might send this message before this handler has been registered, but that's ok, because the changes will get picked up when we call WebAppInternals.reloadClientPrograms for the first time (which happens later).

} else {
runLog.log(`Finished delayed build of ${arch} in ${
new Date - start
}ms`, { arrow: true });

This comment has been minimized.

@benjamn

benjamn Jul 5, 2018
Author Member

Fun fact: sometimes this message appears before => Meteor server restarted, but that's because the legacy build happens in the parent (build) process, whereas server startup happens in the child (server) process, totally in parallel, and the legacy build often takes less time than server startup. In other words, when the legacy build wins this race, the work involved fits entirely within a period of time when the build process was previously doing nothing but waiting for the server to start up.

@hwillson
Copy link
Member

@hwillson hwillson commented Jul 5, 2018

This is awesome @benjamn! One small thing:

  1. meteor create test-app
  2. Started the app and accessed it via http://localhost:3100/. So far so good.
  3. Added a console.log('test'); to client/main.js.
  4. Saw the proper rebuild messages in the console:
=> Client modified -- refreshing
=> Finished delayed build of web.browser.legacy in 233ms
  1. Noticed that http://localhost:3100/ did not autoupdate; the newly added console.log didn't run.
  2. If I manually refresh the browser, it then shows.
  3. Subsequent changes from that point forward rebuild and are autoupdated in the browser (so any changes to the console.log then show automatically).
@benjamn
Copy link
Member Author

@benjamn benjamn commented Jul 5, 2018

@hwillson Great find! I've tracked this down to the code in webapp where we calculate client hashes:

WebApp.clientHash = function (archName) {
archName = archName || WebApp.defaultArch;
return calculateClientHash(WebApp.clientPrograms[archName].manifest);
};
WebApp.calculateClientHashRefreshable = function (archName) {
archName = archName || WebApp.defaultArch;
return calculateClientHash(WebApp.clientPrograms[archName].manifest,
function (name) {
return name === "css";
});
};
WebApp.calculateClientHashNonRefreshable = function (archName) {
archName = archName || WebApp.defaultArch;
return calculateClientHash(WebApp.clientPrograms[archName].manifest,
function (name) {
return name !== "css";
});
};

The problem is that WebApp.defaultArch is exactly the architecture that we delayed rebuilding:
// Though we might prefer to use web.browser (modern) as the default
// architecture, safety requires a more compatible defaultArch.
WebApp.defaultArch = 'web.browser.legacy';

Since the client hash is the same for web.browser.legacy as before, we don't trigger the autoupdate. Seems like this hashing logic should be updated to consider all client bundles, not just WebApp.defaultArch.

benjamn added 4 commits Jul 6, 2018
This means the autoupdate package is no longer responsible for recomputing
client hashes, and we can recompute the hashes whenever there's a new (or
updated) client program, which enables delayed builds of architectures
like web.browser.legacy (#10055).
Now that we're postponing the legacy build until after the first client
refresh message is sent, there's a risk that changes to the legacy build
will not be picked up until after the next rebuild.

If we attempted to fix that problem by sending the refresh message after
the legacy bundle is rebuilt, then we would lose most of the benefit of
delaying the legacy build, because the client would not refresh until
after the legacy build completed.

The right way to fix the problem is by sending a second client refresh
message after the legacy build finishes, but doing so with the current
autoupdate implementation would very likely cause modern clients to reload
a second time.

The solution implemented by this commit is simple in theory: the
autoupdate package should keep track of distinct versions for each client
architecture, so that modern clients will refresh only when the modern
versions change, and legacy clients will refresh only when the legacy
versions change, which allows us to send two refresh messages without
causing any clients to refresh more than once.

In reality, this was a fairly major rewrite, since the ClientVersions
collection has a totally different schema now. I've tested it as well as I
can, though I'm not entirely sure what will happen if clients using the
previous version of the autoupdate package begin receiving DDP messages
from this version of the autoupdate server code.
Also removed the AppRunner#_refreshing boolean hack, since reporting
errors during IPC communication seems desirable.
@benjamn
Copy link
Member Author

@benjamn benjamn commented Jul 7, 2018

Although tests are passing after my rewrite of the autoupdate package, I'm still concerned about the lack of coordination between the webapp package reading the contents of the .meteor/local/build/programs/web.browser.legacy bundle during server startup (in the child process), and the build process rewriting the contents of the web.browser.legacy bundle (also during server startup, but from the parent process).

I can make the delayed writing of the legacy bundle "more atomic" by populating a fresh temporary directory and then renaming that directory, but that's slower overall because it means rewriting everything rather than just the files that have changed. Writing the changed files in place (and deleting any files that were removed) is fundamentally less instantaneous than renaming a directory, but much faster because it doesn't require creating a temporary directory from scratch.

Here are some ideas I'm considering to address this read/write race:

  • Temporarily rename the whole existing .meteor/local/build/programs/web.browser.legacy directory to a temporary location, write any changed files, and then rename it back. The webapp package could then skip loading the legacy bundle if the directory is missing, trusting that it will be loaded later once the bundle has been written. Problems with this approach:

    • The webapp package could start reading the legacy bundle before the build process has a chance to rename it, and I'm not exactly sure how to make the build process wait for the webapp package to finish reading the bundle.
  • Start writing the web.browser.legacy build even later, after the webapp package has finished reading the (stale) bundle, so that there's no overlap. The good news here is that we only need to delay the writing of the bundle, whereas the expensive computation of the bundle contents (makeClientTarget) can begin as early as we like. Problems:

    • The legacy build finishes even later.
  • Use some sort of file system synchronization such as lockfile. Problems:

    • This package (and related packages) appear to be based on the O_EXCL flag, which doesn't work on all file systems (e.g. network file systems like NTFS).

I'm going to keep thinking about this until I have more confidence in one solution over the others.

@KoenLav
Copy link

@KoenLav KoenLav commented Jul 8, 2018

I think building the bundle a bit later still would be preferable as this might even further speed up the process of reloading the page, it is in line with the original logic of this feature and does not require any significant extra code.

benjamn added 5 commits Jul 9, 2018
This package is already importable because it's a dependency of request,
npm, and http-signature, but it's a good idea to depend on it explicitly
just in case those packages stop depending on it in the future.
Instead of having every message consumer listen to every message and act
on the ones that seem relevant to its interests, we now have a single
process.on("message", callback) hook that can dispatch messages to
different Meteor packages running in the server process.

Receiving packages should export an onMessage function. The onMessage
function may be async, and its result will be delivered back to the build
process as the result of the sendMessage Promise.
benjamn added 2 commits Jul 10, 2018
This is the solution I came up with for the problems I described here:
#10055 (comment)
// should regenerate the client program for this arch.
runLog.log(`Finished delayed build of ${arch} in ${
new Date - start
}ms`, { arrow: true });

This comment has been minimized.

@benjamn

benjamn Jul 11, 2018
Author Member

Need to think about whether this logging should remain in the final version. Alternatively, we could add more information about building other bundles, so that this isn't the only message.

@benjamn benjamn force-pushed the wip-delay-building-legacy-bundle branch to 05ffaa5 Jul 11, 2018
@benjamn benjamn merged commit c82141c into release-1.7.1 Jul 11, 2018
19 checks passed
19 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
// package to call Package._define(packageName, exports).
promises[packageName] = (
promises[packageName] || new Promise(resolve => {
Package._on(packageName, resolve);

This comment has been minimized.

@benjamn

benjamn Jul 11, 2018
Author Member

This pattern causes a bug when the autoupdate package (or any other package that receives messages) is not installed, which means Package._on("autoupdate", resolve) will never call resolve, so the build process stops receiving messages from the server process and awaits forever.

benjamn added a commit that referenced this pull request Jul 11, 2018
#10055 (comment)

As I explained in this comment, Package._on(packageName, callback) was a
bad API because it never called the callback if the package was not
installed, which caused any app not using the autoupdate package to get
stuck trying to communicate with the autoupdate package.
benjamn added a commit that referenced this pull request Jan 15, 2019
This partially reverts commit 99b79dc,
which was added as part of PR #10055 in an effort to trigger hot reloads
on the client when/if the definition of a "modern" browser happened to
change, due to server code calling setMinimumBrowserVersions. Although
changes in the minimum modern browser versions are pretty rare, it seemed
important to incorporate this information into the client hash, because
code sent to the client tends to be dramatically different depending on
whether the client is considered modern.

However, this change was made without updating the corresponding version
calculations in CordovaBuilder#appendVersion in tools/cordova/builder.js,
so the versions in program.json for Cordova apps disagreed with the
versions served in manifest.json by the web server, leading to the
problems described by @lorensr in this cordova-plugin-meteor-webapp issue:
meteor/cordova-plugin-meteor-webapp#69

It would be nice to include the minimum versions hash in program.json for
Cordova builds, but unfortunately these versions are not known at build
time, because they are determined by calls to setMinimumBrowserVersions
during server startup. In other words, if we wanted to access that
information during Cordova builds, we would have to start the web server
and run all server-side application initialization code just to find out
if setMinimumBrowserVersions was called anywhere.

In the future, we could consider including the minimum versions hash in
manifest.json, so cordova-plugin-meteor-webapp could compare the current
version to the new version whenever it fetches manifest.json. However, I
think simply removing the minimum versions hash from the client version
calculation is a fine solution in the meantime. If a developer needs to
trigger a hot reload because they changed their minimum modern versions,
they should just be sure to change their client code at the same time.
Any change that would normally trigger a client reload will work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.