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

Faillible Log interface #382

Closed
HadrienG2 opened this issue Feb 29, 2020 · 9 comments
Closed

Faillible Log interface #382

HadrienG2 opened this issue Feb 29, 2020 · 9 comments

Comments

@HadrienG2
Copy link

HadrienG2 commented Feb 29, 2020

It has recently come through my attention, through the joy of low-level programming, that the Log trait does not provide any clean way to propagate I/O errors. Therefore, when one of those happens, a Log implementation must pick between a number of bad choices:

  • Panicking, which in a no_std UEFI context that cannot afford unwinding means an instant crash.
  • Transmitting a subset of the error information through a weird errno-style global side channel, with all the problems that these sorts of side channels bring.
  • Retrying, which is not guaranteed to succeed and will lead to redundant output since UEFI text output is not transactional.
  • Ignoring the error altogether, which while probably least bad still doesn't sound right.

As a superior alternative, could you consider following the example of fmt::Write and providing facilities for propagating the information that a text output error occurred in the Log trait?


Since not wanting to panic on failure is arguably a no_std edge case, it's okay if those facilities are not as ergonomic as the current ones. For example, I could live a backwards-compatible extension to the current Log trait like this one...

pub trait Log: Sync + Send {
    // NOTE: Associated type design may not work for a global logger,
    //       may need something like `fmt::Error` instead
    type Error = !;

    fn log(&self, record: &Record);

    fn log_faillible(&self, record: &Record) -> Result<(), Self::Error> {
        Ok(self.log(record))
    }

    // ...
}

...combined with a "faillible" submodule of the log crate that contains faillible variants of the logging macros exposed at the top level of the module.


With this, we would then be able to advise uefi-rs users to use the faillible logging macros, while still supporting the use of unmodified Rust crates that call the old infaillible macros.

Such a faillible logging trait + macros is probably what we'll end up implementing on the uefi-rs side, in one form or another, if you're not interested. But I thought the idea of propagating I/O errors was of sufficiently general use, at least in no_std use cases, to warrant at least some upstream discussion on your side.

@sfackler
Copy link
Member

As you noted, an associated type isn't going to work with this crate.

What action would you expect code to take in response to an error from a logging statement? Logging is typically used as a backchannel for diagnostic information, and it isn't really typical in my experience to care more about your logging information than the actual computation you're trying to perform. You can't log the fact that your logger doesn't work, and unless you're going to cause the main computation to fail, there aren't really any other clear options, right?

@HadrienG2
Copy link
Author

HadrienG2 commented Feb 29, 2020

Well, as usual with Result-based designs, the idea is to let the caller pick their poison:

  • If they would rather crash when logging fails, they can .unwrap().
  • If they would rather try harder in spite of partial output polluting the console, they can retry a few times before giving up and going for another strategy.
  • If they would rather print a message saying that logging has failed and cross fingers that this one will get through (after all, the failure is intermittent), power to them.
  • If they want to do crazy things with statics like buffering logs in RAM for a later OS initialization stage, I don't want to do it myself but I don't see why I should prevent them from doing it.
  • If they would rather ignore the failure entirely, they can mem::forget() the whole thing (or use .unwrap_or_default() as a somewhat more elegant if more obscure syntax).

@sfackler
Copy link
Member

I agree that those are all things that someone could hypothetically do, but I'm asking why someone would in practice want to worry about any of that, and if that's worth a breaking change to the 6th most used crate in the entire ecosystem.

@HadrienG2
Copy link
Author

HadrienG2 commented Mar 1, 2020

The heart of the issue, as I see it, is that logging means different things to different people. To give some archetypal examples:

  • In user-facing applications, logging is a secondary service that helps users diagnose problems faster and more precisely than they would be able to with just the direct symptoms. This leads to faster problem solving for users, and to fewer and better bug reports for devs, but if it doesn't work both users and developers can live without it.
  • In background system services, logging is the primary communication channel between software and system administrators, and having it running reliably can be a hard legal requirement. Logger failure must be reported via other available communication channels (e.g. stderr, monitoring systems, pop-up notifications in GUI desktops...) and may or may not require service shutdown.
  • In low-level environments without a proper OS, logging is often the only communication channel that's accessible to users without special equipment, and getting a good user diagnosis of what's going on is particularly important as bugs are often hardware-dependent at this layer. The only cases where it's okay to ignore log failures are experimental code and debugging of said failures.

The problem, of course, is that as library authors we may not know what our libraries will be used for. Especially in a language with decent #[no_std] support like Rust, it is possible for a single library to be used in multiple application domains where the criticality of logging as a service varies widely.

This is where configurable error handling behavior makes sense, in my opinion: it allows library authors who don't know the best course of action to defer the decision of how to handle errors to a layer of the software abstraction stack where someone actually knows how critical the error is to the application domain and how it should best be handled.

Now, so far I've focused on Result-based configurable error handling behavior as that's kind of the standard approach in Rust, but there are other ways to make the error handling behavior configurable, like via a global logger configuration switch for example. Some of these other options may actually be a better fit than Result, which is already a bit too opinionated in the sense that bubbling it up always aborts the current computation. So we should probably discuss the alternatives too.


In any case, I agree with you that this is unlikely to be worth a major breaking change in log. A minor breaking change like adding a separate and optional faillible logging method to the Log trait, on the other hand, seems like a possibly workable compromise, which is why this is what I went for initially.

@sfackler
Copy link
Member

sfackler commented Mar 3, 2020

but there are other ways to make the error handling behavior configurable, like via a global logger configuration switch for example.

That already exists - an application author has the ability to configure a logger globally by installing the global logger. If an application cannot continue running in response to a logging failure, then the application should install a logger that terminates the program in response to a logging failure.

@HadrienG2
Copy link
Author

So, if I try to summarize, your opinion is that the log error handling policy should be global to the application rather than local to libraries, and is therefore best handled by the Log trait implementation, either by picking a Log implementation based on error handling needs or by adding contruction- or run-time switches to Log implementations which allow them to switch between different error handling policies.

From this perspective, the only thing that the log crate could do is to standardize an interface for error handling policy configuration, and you don't think that it is worthwhile to do so at this point in time.

Would you agree that this is a fair assessment?

@sfackler
Copy link
Member

sfackler commented Mar 6, 2020

That sounds accurate.

@HadrienG2
Copy link
Author

Thanks for the clarification! Then maybe let's wait a week or two in case someone else wants to voice another opinion, and if not I think we can close this.

@HadrienG2
Copy link
Author

Alright, I think this matter is settled then. Thanks for the discussion! 😉

EFanZh pushed a commit to EFanZh/log that referenced this issue Jul 23, 2023
* Refactor: Extract new fn `BinFile::check_source_exists`
* Impl new async fn `AutoAbortJoinHandle::flattened_join`
* Impl new fn `Fetcher::fetcher_name`
* Verify that `bin_files` exist in `resolve` stage
   To ensure that the installation stage won't fail because of missing
   binaries.
* Rm unused `MultiFecther`
* Simplify `Future` impl for `AutoAbortJoinHandle`
* Add new variant `BinstallError::CargoTomlMissingPackage`
* Replace `unwrap` in `resolve_inner` with proper error handling
* Make `Fetcher::new` as a regular function
   instead of an `async` function.
* Ret `Arc<dyn Fetcher>` in trait fn `Fetcher::new`
* Refactor `resolve_inner`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
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