Factor out "defer" usage for better error handling

[ggetz]

Cesium uses defer throughout the codebase as a remnant from when we were using when.js. However, the recommended best practice is to use new Promise constructor directly because it makes it much harder to let an exception escape. Functions passed to the construct should only reject the promise, never throw.

#10149 excluded several specs in Cesium3DTilesetSpec, GltfLoaderSpec, TimeDynamicPointCloudSpec, and ModelExperimentalSpec which can be re-enabled once errors are better handled as a result of this change.

[ggetz]

@lilleyse @ptrgags Is there a particular reason why the loaders, like BufferLoader.js have a promise rather than having the load function return a promise? As far as I'm aware, the later is a much more common pattern and would make removing defer much more streamlined.

[lilleyse]

Some of the loaders like GltfDracoLoader and GltfVertexBufferLoader are tied to the update loop and need to be polled every frame to see if the task processor or job scheduler can process the job. That was the reason for using the defer pattern instead of returning a promise right away, and for consistency the other loaders do it that way too. Is there a different way to approach this?

[ggetz]

Ah thanks. Looking at these loaders, two things jump out at me:

• The top-level promise seems to be tracking two operations: the load result and the process result. It may make more sense to track these two operations through separate promises, which may or may not live on the loader itself.
• It seems to me that we could move things around and track the state to make sure things aren't being scheduled in the task processor or job scheduler. Would it be problematic to introduce a new ResourceLoaderState between LOADED and PROCESSING? If so, there are other ways to do it, that just seems like a logical place to put a state for "loaded, but waiting to be processed".

[lilleyse]

Separate promises + a new state could be ok. Would this move some of the burden to the caller to load and then process the loader?

Model.js seems like it will have a similar problem and I'm curious how we'll approach that.

[ggetz]

I'm not entirely sure yet, but I don't think so. So far it seems it can be handled internally and we just need to move around where the caller is actually handling the promises.

I agree Model.js will have similar problems. What do you think the timeline is for removing it? Maybe we can leave defer there until its replaced by ModelExperimental. It's not causing issues in the specs like the loaders are currently.

[lilleyse]

I'm not entirely sure yet, but I don't think so. So far it seems it can be handled internally and we just need to move around where the caller is actually handling the promises.

Ok that doesn't sound too bad. I was imagining that a larger restructuring would be needed.

I agree Model.js will have similar problems. What do you think the timeline is for removing it? Maybe we can leave defer there until its replaced by ModelExperimental. It's not causing issues in the specs like the loaders are currently.

We're aiming to remove it in June. Holding off until ModelExperimental replaces it sounds good.

[mramato]

FYI, we ran into issues in unit testing a Cesium-based app related to this behavior and had to insert model.readyPromise.catch(() => {}); into some spots in the tests in order to ensure it doesn't break the entire testing suite when it resolves (simply awaiting doesn't work).

What's weird here is the model is never actually added to the scene, and my current understanding of how this works means that the promise should never reject or resolve.

Ultimately, I think there is more going on here than we realize. For example, destroy doesn't seem to actually be aware that anything async could be happening and I don't see checks in Model after async operations, such as fetchArrayBuffer, to verify the model hasn't been destroyed and turn it into a no-op.

@lilleyse I don't know anything about the ModelExperimental implementation, but hopefully it properly tracks async operations and accounts for things like the user destroying the object while it has an async operation pending and gracefully cancels or ignores the no longer needed operation when it completes.,

We're aiming to remove it in June. Holding off until ModelExperimental replaces it sounds good.

Does that mean the July 1st release, or the June 1st release? It's only April 10th and I think we've introduced or magnifies several promise-related issues in CesiumJS with the last release. Just waiting around and hoping ModelExperimental fixes them seems like a bad call.

[lilleyse]

@lilleyse I don't know anything about the ModelExperimental implementation, but hopefully it properly tracks async operations and accounts for things like the user destroying the object while it has an async operation pending and gracefully cancels or ignores the no longer needed operation when it completes.

The new implementation should be handling destroy + async operations more gracefully. All the loaders follow the pattern of checking isDestroyed after the promise resolves or rejects (for example see GltfTextureLoader.js). ModelExperimental itself doesn't have async operations. It all happens through the loader.

Does that mean the July 1st release, or the June 1st release? It's only April 10th and I think we've introduced or magnifies several promise-related issues in CesiumJS with the last release. Just waiting around and hoping ModelExperimental fixes them seems like a bad call.

It would be the July 1st release. Since this is starting to impact development we should fix Model sooner, assuming it's not a huge effort.

[ggetz]

@sanjeetsuhag What's left here after #10523 and #10496? Is it only to refactor Resource.js? That should likely be its own issue, and we should close this one.

[sanjeetsuhag]

@ggetz 3 areas remain:

• Resource.js and everything related to that.
• TaskProcessor.js - being addressed here: Removes defer usage from TaskProcessor #10545 (review)
• DeferredLoading in KmlDataSource - not sure if worth doing right now, that entire system could probably be reworked. I'll open a separate issue for that too.