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

Better error handling #119

Open
AuHau opened this issue Nov 26, 2019 · 4 comments
Open

Better error handling #119

AuHau opened this issue Nov 26, 2019 · 4 comments

Comments

@AuHau
Copy link
Contributor

AuHau commented Nov 26, 2019

The error handling should be improved as the Errors do not really say much about the problems that they represent.

For example, calling bzz.list(hash) on a hash that is not manifest results in "Internal Server Error" that does not help at all.

@PaulLeCam
Copy link
Collaborator

Could you open this as an issue on Swarm itself please, maybe as a SWIP?
Currently errors in Erebos just use the status text because as far as I know (based on the current docs) Swarm doesn't provide more detailed error messages, but if it does I agree they should be handled by Erebos.

Regarding the example of bzz.list(hash), I don't see how this could be handled client-side anyways, AFAIK there's no way to know if a hash matches a manifest or not without requesting it, so you could either have the server return a 4xx error with an informative error message, or a predefined error code maybe (that could be defined in a SWIP) that could be returned by Erebos, or the application could handle this itself by doing a bzz-raw request to ensure the hash references a manifest.

@AuHau
Copy link
Contributor Author

AuHau commented Nov 27, 2019

I agree that Swarm could improve on its error reporting capabilities, but I think there is already a lot possible actually without need to touch Swarm itself.

For example with the bzz.list() method you actually get message like error loading manifest 92672a471f4419b255d7cb0cf313474a6f5856fb347c5ece85fb706d644b630f: Manifest 92672a471f4419b2 is malformed: invalid character 'h' looking for beginning of value. This message is thrown away as only status code and status text are used and that makes it very hard to debug. That could be the actual first level of improvement.

Also, I think that as an abstraction library it should be avoided to expose the transport errors (eq. HTTPError) as it is not relevant to the user at all. The library should instead interpret these errors to more informative versions that the user can easily catch and treat. Again with the example of bzz.list() upon receiving 500 error with a message like Manifest 92672a471f4419b2 is malformed I would throw something like new InvalidManifestError('Manifest 92672a471f4419b2 is malformed'). With this user can easily catch this error and if it makes sense to him, he can then try to fetch the data using bzz-raw.

@PaulLeCam
Copy link
Collaborator

I agree these could be improved, but the first step should be that the possible error messages need to be specified and documented by Swarm. I don't want to be in the situation of trying to be "too smart for our own good", where the client is trying to interpret error messages and breaks once they change in a new server release.

The fact that Swarm returns a 500 error code rather than a 4xx seems like a missing error handler on the server side to me.
The client shouldn't have to care or at least interpret 5xx errors, that are server-related.

That's why I think creating a SWIP is the best way to start the process, so we can have clear expectations on the client side about what types or error can be returned (HTTP status codes), how to parse them (ex a 400 should be parsed as JSON and contains a Swarm-specific error code and message, while a 404 doesn't have a response body) and then implement the necessary logic in Erebos.

@AuHau
Copy link
Contributor Author

AuHau commented Nov 28, 2019

Well I guess makes sense. Agree about the 500.

There already seems to be an endeavor up in this direction ethersphere/swarm#1601 Let see what will be the result.

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