RFC for adding Sync to io::Error

[lilyball]

Rendered

A PR for this has already been written as rust-lang/rust#24133.

[reem]

This seems to be a sort of general abstraction leakage when using trait objects - there's no way to add bounds to internal trait objects without changing the definition of the type. This is particularly problematic with marker traits: for instance, I can easily see this coming up with Reflect in the future, and it will be technically backwards incompatible to change.

[alexcrichton]

I'm a little hesitant about the motivation here as there seem to be two separate points that are seem to be mixed up a bit:

• Having io::Error be Sync
• Having io::Error be Clone

The second of these motivations is solved with an Rc, which does have the downside of making it not Send. The first seems to have less motivation once io::Error is Clone. For example it doesn't look like the Sync bound is really desired on io::Error other than to put it in an Arc (e.g. are there use cases to share an io::Error concurrently among threads?).

I would personally prefer to see Sync motivated beyond "to put this in an Arc" as Rc seems to go pretty far.

[lilyball]

@alexcrichton The motivation there is "I want to put an io::Error in some other value that itself wants to be Sync". This is a generalization of 'I want to put it in Arc". One example might be some sort of deferred computation (e.g. a future / promise) that wraps a computation returning io::Result<T>. In order to allow for accessing the resulting value without holding a lock, the value needs to be Sync, which means io::Error needs to be Sync.

I don't consider "put it in an Rc" to be an acceptable way to get Clone precisely because it means the containing value is no longer Send.

Also FWIW, I think io::Error really should be Sync based just on the definition of Sync. From the docs:

Types that are not Sync are those that have "interior mutability" in a non-thread-safe way, such as Cell and RefCell in std::cell.

Errors shouldn't have interior mutability (whether thread-safe or not). Calling the various error::Error (or fmt::Debug or fmt::Display) methods on an error should be idempotent. We can't actually guarantee that (as the type could have thread-safe interior mutability), but according to the definition of Sync, the lack of a Sync bound implies interior mutability (rather than the existence of it implying no interior mutability), and that's not something that makes sense for errors.

The only real justification for not having Sync is when the error impl wraps some other value that isn't Sync (typically by having a generic parameter that is instantiated with some non-Sync concrete type). But that should be very rare in practice, and can be handled by adding a custom From impl as necessary in order to convert the error into something suitable for wrapping in io::Error.

[alexcrichton]

Hm, I'm personally still finding it difficult to grasp why we want io::Error to be Sync. It seems like some of the rationale you have is a bit of a red herring maybe?

I want to put an io::Error in some other value that itself wants to be Sync, This is a generalization of 'I want to put it in Arc".

I feel like the generalization in this case is taking it one step too far. The base rationale for this RFC seemed to be a desire to clone an io::Error but it's led into requiring that it have Sync simply for putting it in an Arc, which seems like an odd path to take.

Errors shouldn't have interior mutability (whether thread-safe or not).

As you point out, Mutex can sidestep this, so I don't think that this should be much of a motivation one way or another.

The only real justification for not having Sync is when the error impl wraps some other value that isn't Sync

To me it's also API/implementation detail pollution. We require Send for the clear purpose of sending instances of io::Error across threads, but if we start throwing in traits "because it will apply 99% of the time" then we should have something along the lines of Error + Send + Sync + Reflect + NonManaged + .... There'll need to be a line somewhere in the marker traits implemented here, and I'm trying to grasp which side of the line Sync should be on.

@reem is right in that this is an unfortunate abstraction leakage that we have to deal with this at all, but this is the consequence of using a trait object!

[lilyball]

The base rationale for this RFC seemed to be a desire to clone an io::Error

That's my original motivation, yes. But it's not the only reason to want this behavior. Do you not like the future / promise example? A future is a write-once value. If I were to make a Future type, I'd want to have an API that includes something like

enum FutureResult<T,E> {
    Pending,
    Fulfilled(T),
    Rejected(E)
}

impl Future<T,E> {
    fn get(&self) -> FutureResult<&T,&E>;
}

Since it's write-once, once the value has been written, there's no reason to need any kind of synchronization (internally it would probably use an atomic bool, and once it reads that the bool is true it can just access the fields directly).

More specifically, requiring a Mutex for this case here is completely unnecessary. I shouldn't have to hold a lock in order to access the written data, and I should be able to support concurrent access (either by using Arc or thread::scoped to share it with multiple threads).

But the only way to use this type with E=io::Error (while preserving the Sync bound on FutureResult) is if io::Error is Sync.

We require Send for the clear purpose of sending instances of io::Error across threads

If it's expected for io::Error to be sent across threads, why not let it be shared across threads as well?

but if we start throwing in traits "because it will apply 99% of the time"

That's not the argument I made. My argument was that Sync is defined by what it says about types that don't implement it. If we had a trait NotSync that meant "types that have 'interior mutability' in a non-thread-safe way" (which is a more straightforward definition, but we can't do this because we can't use negative bounds on type parameters), io::Error wouldn't have used it.

When using generics, it makes sense to avoid as many bounds as possible, because each bound further constrains what types can be used with it. But the situation is a bit different with trait objects. By not specifying the Sync bound, io::Error is explicitly opting out of being Sync. And that's very unusual, since nearly all types in Rust are Sync. And because it opts out of being Sync, it forces any type that contains io::Error to also not be Sync (unless the API can be adjusted to use Mutex, but an error-returning API can't do that).

If io::Error requires Sync, any custom error type that isn't Sync can be adapted to work with io::Error with a custom From impl. But if io::Error doesn't require Sync then it outlaws a whole class of otherwise-valid APIs.

[alexcrichton]

Do you not like the future / promise example?

Oh no sorry, I just didn't read it closely enough! This definitely seems like a plausible example.

That's not the argument I made. My argument was that Sync is defined by what it says about types that don't implement it.

Right, but in my mind it's a slippery slope to adding all these trait bounds. I suspect that plausible examples can be contrived for both requiring a marker trait bound as well as requesting that a marker trait bound doesn't exist. For example if I can make an example that means that trait bound X is needed, what's the rationale for not just adding it with the rationale that it likely won't break code in practice?

Largely what I'm getting at is that I just want to make sure that requiring this bound is well motivated beyond "my previous code is no longer compiling", which doesn't sound to be the case. You've proposed some legitimate use cases for requiring Sync.

By not specifying the Sync bound, io::Error is explicitly opting out of being Sync

True, but I think this is a decision that has to be made either way. Types with generics leave the decision up to instantiators, but trait objects either opt-in immediately or opt-out, both of which have pros and cons.

[pythonesque]

What would be an ergonomic solution to the general problem (abstraction leakage)? Is there one? Are there examples of how other languages solve similar problems?

[reem]

One general solution is to enable dynamic (conditional) upcasting, e.g. from Box<Error> to Box<Error + Send>, but this would require changes to the implementation of trait objects.

[pythonesque]

That doesn't seem so nice to me; it solves the problem only at runtime cost.

Actually, I just realized we already sort of support a solution to this by parameterizing over the constraint:

use std::error;
use std::fmt;
use std::thread;

#[derive(Debug)]
struct Foo;

impl fmt::Display for Foo {
    fn fmt(&self, b: &mut fmt::Formatter) -> fmt::Result {
        Ok(())
    }
}

impl error::Error for Foo {
    fn description(&self) -> &str { "" }
}

struct Error<C: ?Sized> where C: error::Error {
    inner: Box<C>
}

type CurrentVersionOfIoError = Error<error::Error + Send>;

fn main() {
    let foo = Error {
        inner: Box::new(Foo) as Box<error::Error + Send + Sync>
    };
    thread::spawn(move || {
        let x = foo.inner.description();
    });
}

Seems awkward to switch to this though, but at least there is a potential solution for future libraries (I honestly had no idea this worked).

[reem]

@pythonesque that's super interesting, but doesn't work for io::Error since its presence in traits means that it needs to have a fully concrete type anyway.

The idea of trait objects is generally to move type-related activities from compile time to run time, so it seems acceptable to just add more dynamic capabilities to solve this problem.

[lilyball]

@pythonesque That's not doable because the whole point of io::Error wrapping custom errors is that it erases the type of the custom error.

[pythonesque]

@kballard It does erase the type of the custom error. The type of foo here is Error<error::Error + Send + Sync>, not Error<Foo>.

@reem The point of trait objects is type erasure, sure, but the only where it has to leak into the runtime. In cases like this, we would still know everything we needed at compile time. I do see your point though, that this is just moving the problem of what bounds io::Error supports from the type definition to the definitions of the traits that return it, meaning it would still be backwards-incompatible to add new bounds; but couldn't making the type of error for those traits an associated type avoid this problem? Or would that break object safety in an unacceptable way?

[lilyball]

@pythonesque Adding a generic parameter to the type and putting the bounds in there doesn't change anything. You've already recognized that it has the same forwards compatibility hazard (and no, associated types don't solve this), and you're still even using a trait object. In fact, your Error type is pretty much identical to just using Box<error::Error + Send> directly.

[reem]

@pythonesque associated types do solve this in concrete cases but make generics and trait objects using these traits rather problematic.

[pythonesque]

@kballard Why don't associated types solve the forward-compatibility problem? Wouldn't that allow each concrete implementation of these traits to decide which constraint to use? And can't the Constraint in Box<Constraint> then be made to include more traits backwards-compatibly?

[lilyball]

@pythonesque I'm not sure exactly what you're proposing when you say to use associated types. As long as io::Error refers to a specific concrete type (or a particular instantiation of a generic type, as in your code), there's no way to add extra bounds without breaking compatibility. I'm guessing you're thinking of something like different traits returning different instantiations of the generic Error type, but those different instantiations would not be compatible with each other, and would not be compatible with io::Error, which defeats the whole purpose of doing this.

In any case, this is getting rather off-track from the point of this RFC.

[reem]

@pythonesque fwiw the original Read and Write traits in new io used associated types for the errors, but it causes a large number of problems (e.g. can't copy from a Read with one error to a Write with a different error).

[pythonesque]

Ah, fair enough :) Then yeah, I'm in favor of this RFC.

[aturon]

I'm in favor of this RFC. In general, the issue of which marker traits to include when using trait objects is a tricky one, but I think a case like this (where a trait object is being stored in an important, concrete type) should err in the direction of including "standard" markers.

(Over time, we're going to need to develop a more robust policy in std; at the moment, we just don't use trait objects in public APIs very often.)

[alexcrichton]

Thanks again for the RFC @kballard! The feedback here is pretty positive and at this time we've decided to merge this! (my concerns have been addressed)