OpenNeo/impress-2020
OpenNeo/impress-2020
1
0
Fork 0
Code Pull requests Activity

Improve movie cancels and error handling

Browse source 
Oops, my inbox was getting full of uncaught promise rejections of `loadImage`!

I'm pretty sure they're caused when multiple images in a movie fail to load (e.g. network problems), but we fail to cancel them. So, the first failure would be caught as a part of `Promise.all` in `loadMovieLibrary`, but then subsequent failures wouldn't be caught by anything, and would propagate up to the console and to Sentry as uncaught errors.

In this change, we make a number of improvements to cancellation. The most relevant change for this bug is that `loadMovieLibrary` will now automatically cancel all resource promises when it throws an error! But this improved robustness also enabled us to finally offer a simple `cancel()` method on movie library promises, which we now available ourselves of at call sites, too.
This commit is contained in:
Emi Matchu 2021-06-26 12:04:40 -07:00
parent 0bffaec989
commit f5e5f16f87
3 changed files with 169 additions and 77 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

219
src/app/components/OutfitMovieLayer.js
View file

@ -124,7 +124,8 @@ function OutfitMovieLayer({
React.useEffect(() => { React.useEffect(() => {
let canceled = false; let canceled = false;
loadMovieLibrary(libraryUrl) const movieLibraryPromise = loadMovieLibrary(libraryUrl);
movieLibraryPromise
.then((library) => { .then((library) => {
if (canceled) { if (canceled) {
return; return;
@ -137,14 +138,25 @@ function OutfitMovieLayer({
}) })
.catch((e) => { .catch((e) => {
console.error(`Error loading outfit movie layer: ${libraryUrl}`, e); console.error(`Error loading outfit movie layer: ${libraryUrl}`, e);
if (!toast.isActive("use-preload-layers-movie-failed")) {
toast({
id: "use-preload-layers-movie-failed",
status: "warning",
title: "Oops, we couldn't load one of these animations.",
description: "We'll show a static image version instead.",
duration: null,
isClosable: true,
});
}
}); });
return () => { return () => {
canceled = true; canceled = true;
movieLibraryPromise.cancel();
setLibrary(null); setLibrary(null);
setMovieClip(null); setMovieClip(null);
}; };
}, [libraryUrl]); }, [libraryUrl, toast]);
// This effect puts the `movieClip` on the `stage`, when both are ready. // This effect puts the `movieClip` on the `stage`, when both are ready.
React.useEffect(() => { React.useEffect(() => {
@ -261,91 +273,150 @@ function OutfitMovieLayer({
} }
function loadScriptTag(src) { function loadScriptTag(src) {
return new Promise((resolve, reject) => { let script;
const script = document.createElement("script"); let canceled = false;
script.onload = () => resolve(script); let resolved = false;
script.onerror = (e) => reject(e);
const scriptTagPromise = new Promise((resolve, reject) => {
script = document.createElement("script");
script.onload = () => {
if (canceled) return;
resolved = true;
resolve(script);
};
script.onerror = (e) => {
if (canceled) return;
reject(e);
};
script.src = src; script.src = src;
document.body.appendChild(script); document.body.appendChild(script);
}); });
scriptTagPromise.cancel = () => {
if (resolved) return;
script.src = "";
canceled = true;
};
return scriptTagPromise;
} }
const MOVIE_LIBRARY_CACHE = new LRU(10); const MOVIE_LIBRARY_CACHE = new LRU(10);
export async function loadMovieLibrary(librarySrc) { export function loadMovieLibrary(librarySrc) {
// First, check the LRU cache. This will enable us to quickly return movie const cancelableResourcePromises = [];
// libraries, without re-loading and re-parsing and re-executing. const cancelAllResources = () =>
const cachedLibrary = MOVIE_LIBRARY_CACHE.get(librarySrc); cancelableResourcePromises.forEach((p) => p.cancel());
if (cachedLibrary) {
return cachedLibrary;
}
// These library JS files are interesting in their operation. It seems like // Most of the logic for `loadMovieLibrary` is inside this async function.
// the idea is, it pushes an object to a global array, and you need to snap // But we want to attach more fields to the promise before returning it; so
// it up and see it at the end of the array! And I don't really see a way to // we declare this async function separately, then call it, then edit the
// like, get by a name or ID that we know by this point. So, here we go, just // returned promise!
// try to grab it once it arrives! const createMovieLibraryPromise = async () => {
// // First, check the LRU cache. This will enable us to quickly return movie
// I'm not _sure_ this method is reliable, but it seems to be stable so far // libraries, without re-loading and re-parsing and re-executing.
// in Firefox for me. The things I think I'm observing are: const cachedLibrary = MOVIE_LIBRARY_CACHE.get(librarySrc);
// - Script execution order should match insert order, if (cachedLibrary) {
// - Onload execution order should match insert order, return cachedLibrary;
// - BUT, script executions might be batched before onloads. }
// - So, each script grabs the _first_ composition from the list, and
// deletes it after grabbing. That way, it serves as a FIFO queue! // Then, load the script tag. (Make sure we set it up to be cancelable!)
// I'm not suuure this is happening as I'm expecting, vs I'm just not seeing const scriptPromise = loadScriptTag(safeImageUrl(librarySrc));
// the race anymore? But fingers crossed! cancelableResourcePromises.push(scriptPromise);
await loadScriptTag(safeImageUrl(librarySrc)); await scriptPromise;
const [compositionId, composition] = Object.entries(
window.AdobeAn.compositions // These library JS files are interesting in their operation. It seems like
)[0]; // the idea is, it pushes an object to a global array, and you need to snap
if (Object.keys(window.AdobeAn.compositions).length > 1) { // it up and see it at the end of the array! And I don't really see a way to
console.warn( // like, get by a name or ID that we know by this point. So, here we go, just
`Grabbing composition ${compositionId}, but there are >1 here: `, // try to grab it once it arrives!
Object.keys(window.AdobeAn.compositions).length //
// I'm not _sure_ this method is reliable, but it seems to be stable so far
// in Firefox for me. The things I think I'm observing are:
// - Script execution order should match insert order,
// - Onload execution order should match insert order,
// - BUT, script executions might be batched before onloads.
// - So, each script grabs the _first_ composition from the list, and
// deletes it after grabbing. That way, it serves as a FIFO queue!
// I'm not suuure this is happening as I'm expecting, vs I'm just not seeing
// the race anymore? But fingers crossed!
if (Object.keys(window.AdobeAn?.compositions || {}).length === 0) {
throw new Error(
`Movie library ${librarySrc} did not add a composition to window.AdobeAn.compositions.`
);
}
const [compositionId, composition] = Object.entries(
window.AdobeAn.compositions
)[0];
if (Object.keys(window.AdobeAn.compositions).length > 1) {
console.warn(
`Grabbing composition ${compositionId}, but there are >1 here: `,
Object.keys(window.AdobeAn.compositions).length
);
}
delete window.AdobeAn.compositions[compositionId];
const library = composition.getLibrary();
// One more loading step as part of loading this library is loading the
// images it uses for sprites.
//
// TODO: I guess the manifest has these too, so if we could use our DB cache
// to get the manifest to us faster, then we could avoid a network RTT
// on the critical path by preloading these images before the JS file
// even gets to us?
const librarySrcDir = librarySrc.split("/").slice(0, -1).join("/");
const manifestImages = new Map(
library.properties.manifest.map(({ id, src }) => [
id,
loadImage(librarySrcDir + "/" + src, {
crossOrigin: "anonymous",
}),
])
); );
}
delete window.AdobeAn.compositions[compositionId];
const library = composition.getLibrary();
// One more loading step as part of loading this library is loading the // Wait for the images, and make sure they're cancelable while we do.
// images it uses for sprites. const manifestImagePromises = manifestImages.values();
// cancelableResourcePromises.push(...manifestImagePromises);
// TODO: I guess the manifest has these too, so if we could use our DB cache await Promise.all(manifestImagePromises);
// to get the manifest to us faster, then we could avoid a network RTT
// on the critical path by preloading these images before the JS file
// even gets to us?
const librarySrcDir = librarySrc.split("/").slice(0, -1).join("/");
const manifestImages = new Map(
library.properties.manifest.map(({ id, src }) => [
id,
loadImage(librarySrcDir + "/" + src, {
crossOrigin: "anonymous",
}),
])
);
await Promise.all(manifestImages.values());
// Finally, once we have the images loaded, the library object expects us to // Finally, once we have the images loaded, the library object expects us to
// mutate it (!) to give it the actual image and sprite sheet objects from // mutate it (!) to give it the actual image and sprite sheet objects from
// the loaded images. That's how the MovieClip's internal JS objects will // the loaded images. That's how the MovieClip's internal JS objects will
// access the loaded data! // access the loaded data!
const images = composition.getImages(); const images = composition.getImages();
for (const [id, image] of manifestImages.entries()) { for (const [id, image] of manifestImages.entries()) {
images[id] = await image; images[id] = await image;
} }
const spriteSheets = composition.getSpriteSheet(); const spriteSheets = composition.getSpriteSheet();
for (const { name, frames } of library.ssMetadata) { for (const { name, frames } of library.ssMetadata) {
const image = await manifestImages.get(name); const image = await manifestImages.get(name);
spriteSheets[name] = new window.createjs.SpriteSheet({ spriteSheets[name] = new window.createjs.SpriteSheet({
images: [image], images: [image],
frames, frames,
}); });
} }
MOVIE_LIBRARY_CACHE.set(librarySrc, library); MOVIE_LIBRARY_CACHE.set(librarySrc, library);
return library; return library;
};
const movieLibraryPromise = createMovieLibraryPromise().catch((e) => {
// When any part of the movie library fails, we also cancel the other
// resources ourselves, to avoid stray throws for resources that fail after
// the parent catches the initial failure. We re-throw the initial failure
// for the parent to handle, though!
cancelAllResources();
throw e;
});
// To cancel a `loadMovieLibrary`, cancel all of the resource promises we
// load as part of it. That should effectively halt the async function above
// (anything not yet loaded will stop loading), and ensure that stray
// failures don't trigger uncaught promise rejection warnings.
movieLibraryPromise.cancel = cancelAllResources;
return movieLibraryPromise;
} }
export function buildMovieClip(library, libraryUrl) { export function buildMovieClip(library, libraryUrl) {

13
src/app/components/OutfitPreview.js
View file

@ -387,12 +387,15 @@ export function usePreloadLayers(layers) {
// Start preloading the movie. But we won't block on it! The blocking // Start preloading the movie. But we won't block on it! The blocking
// request will still be the image, which we'll show as a // request will still be the image, which we'll show as a
// placeholder, which should usually be noticeably faster! // placeholder, which should usually be noticeably faster!
const movieAssetPromise = loadMovieLibrary( const movieLibraryPromise = loadMovieLibrary(
layer.canvasMovieLibraryUrl layer.canvasMovieLibraryUrl
).then((library) => ({ );
const movieAssetPromise = movieLibraryPromise.then((library) => ({
library, library,
libraryUrl: layer.canvasMovieLibraryUrl, libraryUrl: layer.canvasMovieLibraryUrl,
})); }));
movieAssetPromise.libraryUrl = layer.canvasMovieLibraryUrl;
movieAssetPromise.cancel = () => movieLibraryPromise.cancel();
movieAssetPromises.push(movieAssetPromise); movieAssetPromises.push(movieAssetPromise);
// The minimal asset for the movie case is *either* the image *or* // The minimal asset for the movie case is *either* the image *or*
@ -439,7 +442,11 @@ export function usePreloadLayers(layers) {
(alreadyHasAnimations) => alreadyHasAnimations || assetHasAnimations (alreadyHasAnimations) => alreadyHasAnimations || assetHasAnimations
); );
}; };
movieAssetPromises.forEach((p) => p.then(checkHasAnimations)); movieAssetPromises.forEach((p) =>
p.then(checkHasAnimations).catch((e) => {
console.error(`Error preloading movie library ${p.libraryUrl}:`, e);
})
);
}; };
loadAssets(); loadAssets();

14
src/app/util.js
View file

@ -356,9 +356,12 @@ export function loadImage(rawSrc, { crossOrigin = null } = {}) {
const src = safeImageUrl(rawSrc, { crossOrigin }); const src = safeImageUrl(rawSrc, { crossOrigin });
const image = new Image(); const image = new Image();
let canceled = false; let canceled = false;
let resolved = false;
const promise = new Promise((resolve, reject) => { const promise = new Promise((resolve, reject) => {
image.onload = () => { image.onload = () => {
if (canceled) return; if (canceled) return;
resolved = true;
resolve(image); resolve(image);
}; };
image.onerror = () => { image.onerror = () => {
@ -370,10 +373,21 @@ export function loadImage(rawSrc, { crossOrigin = null } = {}) {
} }
image.src = src; image.src = src;
}); });
promise.cancel = () => { promise.cancel = () => {
// NOTE: To keep `cancel` a safe and unsurprising call, we don't cancel
// resolved images. That's because our approach to cancelation
// mutates the Image object we already returned, which could be
// surprising if the caller is using the Image and expected the
// `cancel` call to only cancel any in-flight network requests.
// (e.g. we cancel a DTI movie when it unloads from the page, but
// it might stick around in the movie cache, and we want those images
// to still work!)
if (resolved) return;
image.src = ""; image.src = "";
canceled = true; canceled = true;
}; };
return promise; return promise;
} }