Add response body to network errors #3714
Conversation
This was introduced in fa125b9, and then "reverted" in 1cf4801, except that revert left the struct around doing nothing useful. We're removing it all the way now because we want to make a new `TeeSink` complementing the already-exiting `TeeSource`, that is actually a completely different concept as far as the class hierarchy is concerned.
This is a bit complex because we want to expose extra functionality the wrapped class has. Perhaps there is some inheritancy trickery to do this nicer, but I don't know it, and this is the first thing we tried after a series of attempts that did build. This design is kind of like that of Rust's Writer, Reader, or Iter adapters, which impliment more traits based on what the inner type implements.
Everywhere seems to use `getHTTPStatus` now.
|
Ok, so the response is a member of the FileTransferError exception. Do you plan on the response showing up as part of the message the user sees when the FileTransferError is handled? Or will it be used for something else, like the But I'm guessing the purpose off the response field is something else, in which case LGTM. |
|
I was thinking maybe just show it on higher verbosity levels. I was also reminded of you making |
|
I don't really have a plan as yet for varying amounts of error detail depending on verbosity, but you could do that in the FileTransferError constructor if you're building the hint message there. SysError takes this both-ways approach of saving the errno and embedding it into the hint in its constructor, though without any verbosity checks. |
We have a larger problem that passsing computed strings to the first variable argument of many exception constructors is unsafe because that first variable argument is interpreted not as a plain string, but format string, and if it contains '%' boost::format will abort, since there are no arguments to the format string. In this particular instance '%' was used as part of an escape code in a URL, which, when the download failed, caused Nix to abort displaying the `FileTransferError`.
…to-network-errors
Ericson2314
changed the title
|
Included #3720 as I needed that to be able to verify this. (My best source of 500 errors used escaped |
Ericson2314
changed the title
|
OK I did it just like |
Ericson2314
changed the title
|
I added a bunch more stuff to #3720, so I don't think it makes sense to block this on that just cause they contain a commit in common, hence the rename. |
src/libstore/filetransfer.cc
Outdated
| : Error(args...), error(error), response(response) | ||
| { | ||
| const auto hf = hintfmt(args...); | ||
| if (response) { |
There was a problem hiding this comment.
Only do this under higher verbosity (-vvvv). Or Body is "not HTML", we can use some heuristic for that.
Without enough -vvvv, we can also adjust this message so the user knows whether they can try again and get more info, or there wasn't an error message anyways.
Due to NixOS#3841 we don't know how print different messages for different verbosity levels.
| // FIXME: Due to https://github.com/NixOS/nix/issues/3841 we don't know how | ||
| // to print different messages for different verbosity levels. For now | ||
| // we add some heuristics for detecting when we want to show the response. |
|
OK CI is fixed. |
@bburdette So we've gotten the new field into the error message after much wrangling, but I wasn't quite sure what to do next, and I thought it might be a good time to solicit your advice anyways.