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

Wrapping errors that do not implement Send #51

Open
jaysonsantos opened this issue Mar 17, 2021 · 4 comments
Open

Wrapping errors that do not implement Send #51

jaysonsantos opened this issue Mar 17, 2021 · 4 comments

Comments

@jaysonsantos
Copy link
Contributor

Hi there, first of all, thank you very much for this project. It makes working with errors in cli implementations really pleasant and fun!
I was trying to wrap an error that comes from a library I wrote that uses thiserror:

#[derive(Debug, Error)]
pub enum MambembeKeyringError {
    #[error("password not stored in the keyring yet")]
    NoPasswordFound,
    #[error("deserialization error")]
    DeserializationError(#[from] serde_json::Error),
    #[error("unknown keyring backend error")]
    UnknownBackendError(#[from] KeyringError),
}

Only this should be ok but on UnknownBackendError, that KeyringError can contain a KeyringError(OsError), and on windows, OsError is windows::Error.
I noticed the error when I tried to use ? on a function that uses eyre::Result and the following error showed up:

error[E0277]: `NonNull<c_void>` cannot be sent between threads safely
  --> cli\src\main.rs:71:45
   |
71 |             mambembe_keyring::set(&services)?;
   |                                             ^ `NonNull<c_void>` cannot be sent between threads safely
   |
   = help: within `MambembeKeyringError`, the trait `Send` is not implemented for `NonNull<c_void>`
   = note: required because it appears within the type `windows::interfaces::unknown::IUnknown`
   = note: required because it appears within the type `windows::bindings::windows::win32::winrt::IRestrictedErrorInfo`
   = note: required because it appears within the type `Option<windows::bindings::windows::win32::winrt::IRestrictedErrorInfo>`
   = note: required because it appears within the type `windows::result::error::Error`
   = note: required because it appears within the type `keyring::error::KeyringError`
   = note: required because it appears within the type `MambembeKeyringError`
   = note: required because of the requirements on the impl of `From<MambembeKeyringError>` for `ErrReport`
   = note: required by `from`

And my first thought was to wrap it with eyre using the suggestions here:

fn wrap_keyring_error(e: MambembeKeyringError) -> Report {
    // This is done because windows::Error does not allow Send
    eyre!(e)
}

so I could write:

mambembe_keyring::set(&services).map_err(wrap_keyring_error)?;

but the following error returns:

error[E0599]: no method named `eyre_kind` found for reference `&MambembeKeyringError` in the current scope
   --> cli\src\main.rs:177:5
    |
177 |     eyre!(e)
    |     ^^^^^^^^ method not found in `&MambembeKeyringError`
    | 
   ::: C:\Users\....\mambembe\keyring\src\lib.rs:17:1
    |
17  | pub enum MambembeKeyringError {
    | -----------------------------
    | |
    | doesn't satisfy `MambembeKeyringError: Into<ErrReport>`
    | doesn't satisfy `MambembeKeyringError: Send`
    | doesn't satisfy `MambembeKeyringError: Sync`
    | doesn't satisfy `_: color_eyre::eyre::private::kind::TraitKind`
    |
    = note: the method `eyre_kind` exists but the following trait bounds were not satisfied:
            `MambembeKeyringError: Into<ErrReport>`
            which is required by `MambembeKeyringError: color_eyre::eyre::private::kind::TraitKind`
            `MambembeKeyringError: Send`
            which is required by `&MambembeKeyringError: color_eyre::eyre::private::kind::AdhocKind`
            `MambembeKeyringError: Sync`
            which is required by `&MambembeKeyringError: color_eyre::eyre::private::kind::AdhocKind`
            `&MambembeKeyringError: Into<ErrReport>`
            which is required by `&MambembeKeyringError: color_eyre::eyre::private::kind::TraitKind`
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

Do you see a solution for that?
Thanks in advance!

@jaysonsantos
Copy link
Contributor Author

I think this will be mostly for documentation as windows::Error will support Send and Sync soon microsoft/windows-rs#601

@yaahc
Copy link
Collaborator

yaahc commented Mar 17, 2021

I'll do some research on how we could approach this. I'm thinking either provide an alternate constructor that wraps the errors in a Mutex to make them send and sync and documents how this impacts downcasting or provide a non send / sync version of eyre::Report that works with all error types but doesn't implement Send or Sync itself.

If you have suggestions on which of these you'd prefer please lmk.

@jaysonsantos
Copy link
Contributor Author

Hey @yaahc thanks for the answer!
Thinking on what you've said, I tried to wrap the error that is not synced and it indeed works.

#[derive(Debug)]
pub struct KeyringWrapper(Arc<Mutex<KeyringError>>);

impl StdError for KeyringWrapper {
    fn source(&self) -> Option<&(dyn StdError + 'static)> {
        todo!()
    }
}

impl Display for KeyringWrapper {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        self.0.lock().unwrap().fmt(f)
    }
}

But I was not sure on what to put on source 🙈 and committed with a todo for now.
I wish I could be more helpful on that but I cannot think of any good idea.

@yaahc
Copy link
Collaborator

yaahc commented Mar 18, 2021

But I was not sure on what to put on source see_no_evil and committed with a todo for now.
I wish I could be more helpful on that but I cannot think of any good idea.

The only way I can think of to do this is to return another newtype that still wraps the entire KeyringError error but has some indicator of which error in the chain it should be masquerading as, so then your Display impl then locks the guard and starts iterating over sources to the correct error in the chain to masquerade as... 😬

pksunkara pushed a commit that referenced this issue Oct 11, 2023
* Implement panic_note feature

* Fix typo

* Rename to panic_section and use Display impl

* Update src/config.rs

* Move panic_section after panic message

Co-authored-by: Jane Lusby <jlusby42@gmail.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