Closed kring closed 3 months ago
One easy improvement idea is to delete the AssetFetcher
copy/move constructors and assignment operators. That will allow the efficiency of storing these objects by reference in AssetFetcher
, and the convenience of keeping them in an object rather than passing them as individual parameters, while also forcing implementors of converter functions to do the safe thing of capturing individual fields by value rather than capturing the entire object by value (which inadvertently captures the async system and headers by reference).
This has been addressed in #912 .
I'm going to reopen this, because we generally don't close issues until the PR that addresses them is merged. If you add "Fixes #893" to the top post of the PR, then GitHub will automatically close the issue upon merge.
I'm going to reopen this, because we generally don't close issues until the PR that addresses them is merged. If you add "Fixes #893" to the top post of the PR, then GitHub will automatically close the issue upon merge.
Whoops! No problem.
The new
AssetFetcher
introduced in #854 stores theAsyncSystem
and the vector of request headers by reference. This is safe if at least one of these is true:AssetFetcher
is only used during the course of the function it is passed to, never after the function returns. But this is not true.I3dmToGltfConverter
captures it in a lambda to be used later, after the function returns. Even if we fix this converter, future converters will need to keep this in mind as well.The current use may very well be safe (even checking that is a fair bit of work), but it's precarious. It's easy to imagine future changes leading to subtle, intermittent bugs.
The easiest solution is store the AsyncSystem and request headers by value instead. If that's too expensive, the next best thing is to pass them to the function directly rather than wrapping them up in
AssetFetcher
. It makes for a lot of parameters, but it's much more natural / obvious to capture them by value that way.CC @timoore