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

ProxyTool#store should not try multiple tools on server errors #167

Open
paulkaplan opened this issue Sep 2, 2020 · 2 comments
Open

ProxyTool#store should not try multiple tools on server errors #167

paulkaplan opened this issue Sep 2, 2020 · 2 comments

Comments

@paulkaplan
Copy link
Contributor

The ProxyTool#store method should only try one of the registered tools if it gets a non-200 response from the server. The cascade should only be used if there is an error thrown related to creating the request, not from server errors.

This is causing multiple failing requests to be issued when storing an asset, even when the asset is rejected from the server (e.g. 400, 413, 403).

/cc @cwillisf

@paulkaplan
Copy link
Contributor Author

@cwillisf I'm not sure the best way to address this, we seem to be caught in a bit of a catch 22, since we want bubble server errors as promise rejections to the consumer of scratch-storage, but want to differentiate server vs. non-server errors in order to try multiple tools. Should #store just not cascade?

Also to be clear about the impact, it isn't exactly a blocker to scratchfoundation/scratch-gui#6073, but it would double the time to seeing those errors since it will try every failed asset save twice. There is plenty of other work on the GUI side left before this becomes a blocker though.

@apple502j
Copy link

@paulkaplan I'm a bit confused. ProxyTool does not have store, and there is no "multiple tools" to be concerned, AFAIK.

There are a few things that we call "tool" - but, none of it makes sense.

  • It could refer to tools used to send requests. However, since NestTool was removed and FetchWorkerTool doesn't implement POST requests, there should only be one tool used when saving assets.
  • It could refer to methods used to store the asset ("helper"). However, there are only two helpers, WebHelper and BuiltinHelper. BuiltinHelper does not require network though, only WebHelper sends requests.
  • It could refer to tools used by WebHelper.store. However, the code makes sure asset tool is not used for projects, and vice versa.
  • It could refer to websites to store the asset ("store"). However, if there are multiple choices, it always tries the first one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants