heroku / buildpacks-python

Heroku's Cloud Native Buildpack for Python applications.
BSD 3-Clause "New" or "Revised" License
27 stars 3 forks source link

Consider using `fs-err` to improve I/O error messages #270

Open edmorley opened 2 months ago

edmorley commented 2 months ago

The fs-err crate aims to improve error messages for I/O errors by eg including the paths of the file/... that triggered the error: https://crates.io/crates/fs-err

It's supposed to be a drop-in replacement, however, testing locally I found that it made the error message worse in some cases.

For example, before switching to fs-err, the output from the runtime_txt_io_error integration test (whose fixture intentionally has runtime.txt that contains invalid unicode) was:

[Error: Unable to read runtime.txt]
An unexpected error occurred whilst reading the (optional) runtime.txt file.

Details: I/O Error: stream did not contain valid UTF-8

And after:

[Error: Unable to read runtime.txt]
An unexpected error occurred whilst reading the (optional) runtime.txt file.

Details: I/O Error: failed to read from file `/workspace/runtime.txt`

With the error message temporarily changed to print the debug output, we can more clearly see the issue:

[Error: Unable to read runtime.txt]
An unexpected error occurred whilst reading the (optional) runtime.txt file.

Details: I/O Error: Custom { kind: InvalidData, error: Error { kind: Read, source: Error { kind: InvalidData, message: "stream did not contain valid UTF-8" }, path: "/workspace/runtime.txt" } }

ie: fs-err wraps the underlying cause of the io::Error, and that cause is not included in the display representation of the top level error. Instead, one has to manually check for and print error.source(). This is apparently by design (which I get, but it still means more work to use fs-err, and it not quite being the drop-in replacement it promises): https://github.com/andrewhickman/fs-err/issues/51

It may still be worth using fs-err - however, if we adopt it then further buildpack changes will be required to maintain all error message content (such as having the log_io_error function print a "caused by" line iff error.sources() is Some).

edmorley commented 2 months ago

Filed https://github.com/andrewhickman/fs-err/issues/59.

edmorley commented 1 week ago

Filed andrewhickman/fs-err#59.

This was fixed in https://github.com/andrewhickman/fs-err/pull/60 which was released in v3.0.0 of fs-err: https://github.com/andrewhickman/fs-err/blob/main/CHANGELOG.md#300