Possible fix for #5032: Don't hang forever on asset loading error #5033

diarmidmackenzie · 2022-03-22T14:24:08Z

Description:

Currently, if a bad URL is specified for an asset, A-Frame just shows the loading screen indefinitely (regardless of the asset timeout configured).

This prevents applications from providing application-specific handling (e.g. their own error message) when this might happen.
(in terms of why this might happen, suppose the HTML content is generated by server-side rendering on a back-end, and a bug emerges in the backend).

Changes proposed:

Update a-node processing when awaiting all children loading, so that:

it acknowledges the possibility of errors, and handles them
goes ahead and declares items "loaded" so that the scene can render, even if there have been errors.

I also added a small example showing the problem:
/examples/test/gltf-model-error/

Not sure if you'll want this included as part of the PR, but it was handy for reproducing the problem & validating the fix.

dmarcos · 2022-03-23T04:30:48Z

src/core/a-node.js

+          // Failed to load a node, but things won't get any better by waiting around.
+          // So get on with rendering the scene by signaling that this node has loaded.
+          // An "error" event has already been fired, so we don't need to fire another one.
+          warn('Rendering scene with errors on node: ', self);


Don't you think this can be misleading? Worried that we're masking errors that the dev might want to stop and fix and could go unnoticed.

There's already a console error about the failure to load a file.
So it didn't seem to me like another error was helpful, and a warning was the appropriate error level here.

Is there anything we can do apart from console warnings & errors to draw a Dev's attention to the fact that something is not right?

(apart from failing to render the scene at all, which as per #5032 I don't think that's a good solution).

dmarcos · 2022-03-23T04:32:06Z

examples/test/gltf-model-error/index.html

+  <head>
+    <meta charset="utf-8">
+    <title>glTF Model</title>
+    <meta name="description" content="glTF Model - A-Frame">


I know it's misleading the test examples are not actually tests but simple examples that test very specific features. For this we should probably write a unit test

OK - I will have a go at a Unit Test.

Unit Tests now added.

Can we leave this in as well, or do you prefer me to delete it?

I would remove this so examples/test is consistent. It serves the purpose to test visually specific features. Your unit test handles the failure case.

OK - removed it.

diarmidmackenzie · 2022-05-03T14:30:34Z

Sorry for the slow progress on this.

I've now added some test cases.

It took me a long time to realize that Mocha is very sensitive to detecting unhandled errors, and will fail tests whenever your code emits an event named "error", and your test script does not immediately call done(), or stop event propagation.

The error message for this is rather opaque...

Error: [object CustomEvent] (undefined:undefined)

I have also adjusted the fix itself. Previously, as soon as a single faulty asset was found, it would jump into the scene. Now it proceeds with the rest of asset loading on the normal pattern, and logs warnings about the failed assets at the end.

I think that's better behaviour: as per the original issue, we want to mitigate the impact of one incorrect asset URL, but that doesn't mean we want to jump into rendering the scene before other, available, assets have had a chance to load.

diarmidmackenzie · 2022-05-03T14:33:04Z

I've left the gltf_model_error.html test file in for now, as I found it pretty useful while working on this, so it could be useful in future?

But happy to remove it if you prefer...

src/core/a-node.js

diarmidmackenzie · 2022-09-15T20:45:43Z

@dmarcos, with the UTs done, can this be merged now? Or are there other concerns to address?

@kylebakerio has reached out to ask whether I can push ahead and get this merged.

tests/core/a-assets.test.js

vincentfretin · 2022-09-28T09:12:59Z

@dmarcos you should probably merge this first, then I will rebase #5123 to resolve a conflict.

vincentfretin · 2022-09-30T06:54:17Z

@diarmidmackenzie can you please rebase your PR, you need to fix the conflict around evt.stopPropagation(); since #5123 was merged first.

diarmidmackenzie · 2022-09-30T16:20:45Z

@diarmidmackenzie can you please rebase your PR, you need to fix the conflict around evt.stopPropagation(); since #5123 was merged first.

Done.

dmarcos · 2022-11-17T21:50:46Z

This needs a rebase. Combing through open stuff to prep for a 1.4.0 release. Thanks for the patience

diarmidmackenzie · 2022-11-18T17:30:19Z

I believe this is now all up-to-date with latest master again.

A couple of conversation points where I'm not 100% sure whether you were happy with what I'd proposed or not..
#5033 (comment)
#5033 (comment)

diarmidmackenzie · 2022-11-18T17:31:46Z

Hmm - seing a test failure like this:
SUMMARY:
✔ 734 tests completed
ℹ 3 tests skipped
✖ 1 test failed

FAILED TESTS:
✖ "after each" hook for "does not try to enter VR if already in VR"

I'll look into it.

diarmidmackenzie · 2022-11-18T19:13:26Z

OK, I don't know why this test issue has only come to light now, but it is due to inadequate mocking of the canvas element.

This leads to an exception in the look-controls component when it tries to add to the canvas classList.

TypeError: Cannot read properties of undefined (reading 'add')
      at enableGrabCursor (build/commons.js:17060:32)
      at NewComponent.updateGrabCursor (build/commons.js:17074:7)
      at NewComponent.update (build/commons.js:16723:12)
      at NewComponent.initComponent (build/commons.js:26161:10)
      at NewComponent.updateProperties (build/commons.js:26130:12)
      at AEntity.updateComponent (build/commons.js:24971:17)
      at AEntity.updateComponents (build/commons.js:24948:12)
      at entityLoadCallback (build/commons.js:24739:12)
      at emitLoaded (build/commons.js:25673:9)

I believe the solution (which I've just added as a fix into this PR) is to extend the mock canvas object so that classList.add and classList.remove can be invoked without causing exceptions.

I don't have a good understanding of why this didn't cause problems prior to this fix, but the exception does occur in a loading callback, and we have made changes in the handling of such exceptions, so it's plausible that for some reason this exception didn't cause problems for the tests previously.

This particular failng test is not intended to test exceptions, and the intent of the test will be better met if we avoid these exceptions, which is what the extension to the canvas mock object achieves. So I think that is the correct fix, and I don't see any evidence of a problem in the code under test.

kylebakerio · 2022-11-18T21:34:27Z

Vincent has made many changes asnd improvements to the tests in recent months, part of which allowed many tests that were being skipped to now be run. Could be related to that, I didn't look into those changes in too much detail.

Good catch and fix.

vincentfretin · 2022-12-01T08:38:04Z

There is also an error on master with firefox related to classList that is not mocked properly
https://github.com/aframevr/aframe/actions/runs/3520429062/jobs/5901336232
I don't know either why it shows up only now. It may be good to create a PR just for this mocking fix if this PR is not merged right away.

vincentfretin · 2022-12-01T09:16:55Z

I created #5176 with a slightly modified version of the fix @diarmidmackenzie

diarmidmackenzie · 2022-12-01T12:56:19Z

@dmarcos Do you want to take this fix? Or give more feedback on the discussion items above?

vincentfretin · 2022-12-02T09:29:41Z

tests/core/scene/a-scene.test.js

+        classList: {
+          add: function () {},
+          remove: function () {}
+        },


@diarmidmackenzie now that #5176 is merged, you should remove first those 4 lines and then merge changes from master to fix the conflict.

Of course - sorry, didn't immediately register the implications of your comment.

Those updates are done.

This reverts commit 84a0d63.

dmarcos · 2022-12-06T02:51:39Z

We can merge this once we remove the example. Thanks for sticking with it.

diarmidmackenzie · 2022-12-06T18:22:22Z

Ok - removed that file from examples\test so I think we're good now.

dmarcos · 2022-12-06T19:34:04Z

Thanks so much. Super appreciated

diarmidmackenzie added 2 commits March 22, 2022 14:12

Better handling of asset loading errors.

b21a413

Simple test for asset loading error.

a9c2834

dmarcos reviewed Mar 23, 2022

View reviewed changes

Ensure all assets loaded even when one is in error. Plus test cases.

53ff05a

vincentfretin reviewed Sep 11, 2022

View reviewed changes

src/core/a-node.js Outdated Show resolved Hide resolved

vincentfretin reviewed Sep 11, 2022

View reviewed changes

src/core/a-node.js Outdated Show resolved Hide resolved

fix typos

a3dd350

vincentfretin mentioned this pull request Sep 26, 2022

really run all tests and fix broken tests #5123

Merged

vincentfretin reviewed Sep 26, 2022

View reviewed changes

tests/core/a-assets.test.js Outdated Show resolved Hide resolved

diarmidmackenzie added 3 commits September 30, 2022 17:04

Merge remote-tracking branch 'upstream/master' into asset-loading-error

d258b19

Merge remote-tracking branch 'upstream/master' into asset-loading-error

d224351

Remove trailing spaces to pass linting checks.

7f3ba15

vincentfretin mentioned this pull request Oct 6, 2022

Asset loading hangs forever when errors hit loading an asset. #5032

Closed

diarmidmackenzie added 3 commits November 18, 2022 17:15

Merge remote-tracking branch 'upstream/master' into asset-loading-error

c4b9f12

Fix linting errors arising from merge

0f2e977

Fix up merge

2209a49

Fix test script that fails due to inadequate mocking of canvas.

84a0d63

vincentfretin mentioned this pull request Dec 1, 2022

Fix failing tests with firefox by mocking canvas.classList #5176

Merged

vincentfretin reviewed Dec 2, 2022

View reviewed changes

diarmidmackenzie added 2 commits December 4, 2022 15:59

Revert "Fix test script that fails due to inadequate mocking of canvas."

c8a3806

This reverts commit 84a0d63.

Merge remote-tracking branch 'upstream/master' into asset-loading-error

22605bf

Remove example - this is now tested in UT.

37aca83

dmarcos merged commit 5189765 into aframevr:master Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possible fix for #5032: Don't hang forever on asset loading error #5033

Possible fix for #5032: Don't hang forever on asset loading error #5033

diarmidmackenzie commented Mar 22, 2022

dmarcos Mar 23, 2022 •

edited

Loading

diarmidmackenzie Mar 23, 2022

dmarcos Mar 23, 2022

diarmidmackenzie Mar 23, 2022

diarmidmackenzie Sep 30, 2022

diarmidmackenzie Sep 30, 2022

dmarcos Dec 6, 2022

diarmidmackenzie Dec 6, 2022

diarmidmackenzie commented May 3, 2022 •

edited

Loading

diarmidmackenzie commented May 3, 2022 •

edited

Loading

diarmidmackenzie commented Sep 15, 2022

vincentfretin commented Sep 28, 2022

vincentfretin commented Sep 30, 2022

diarmidmackenzie commented Sep 30, 2022

dmarcos commented Nov 17, 2022

diarmidmackenzie commented Nov 18, 2022 •

edited

Loading

diarmidmackenzie commented Nov 18, 2022

diarmidmackenzie commented Nov 18, 2022

kylebakerio commented Nov 18, 2022

vincentfretin commented Dec 1, 2022

vincentfretin commented Dec 1, 2022

diarmidmackenzie commented Dec 1, 2022

vincentfretin Dec 2, 2022

diarmidmackenzie Dec 3, 2022

diarmidmackenzie Dec 4, 2022

dmarcos commented Dec 6, 2022

diarmidmackenzie commented Dec 6, 2022

dmarcos commented Dec 6, 2022

Possible fix for #5032: Don't hang forever on asset loading error #5033

Possible fix for #5032: Don't hang forever on asset loading error #5033

Conversation

diarmidmackenzie commented Mar 22, 2022

dmarcos Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diarmidmackenzie commented May 3, 2022 • edited Loading

diarmidmackenzie commented May 3, 2022 • edited Loading

diarmidmackenzie commented Sep 15, 2022

vincentfretin commented Sep 28, 2022

vincentfretin commented Sep 30, 2022

diarmidmackenzie commented Sep 30, 2022

dmarcos commented Nov 17, 2022

diarmidmackenzie commented Nov 18, 2022 • edited Loading

diarmidmackenzie commented Nov 18, 2022

diarmidmackenzie commented Nov 18, 2022

kylebakerio commented Nov 18, 2022

vincentfretin commented Dec 1, 2022

vincentfretin commented Dec 1, 2022

diarmidmackenzie commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Dec 6, 2022

diarmidmackenzie commented Dec 6, 2022

dmarcos commented Dec 6, 2022

dmarcos Mar 23, 2022 •

edited

Loading

diarmidmackenzie commented May 3, 2022 •

edited

Loading

diarmidmackenzie commented May 3, 2022 •

edited

Loading

diarmidmackenzie commented Nov 18, 2022 •

edited

Loading