Poll evented mutability

[carllerche]

Generally speaking, it is unsafe to access to perform asynchronous
operations using &self. Taking &self allows usage from a Sync
context, which has unexpected results.

Taking &mut self to perform these operations prevents using these
asynchronous values from across tasks (unless they are wrapped in
RefCell or Mutex. This also allows removing synchronization that
really shouldn't be there in the first place. Specifically, an atomic is no
longer used to cache the readiness state. This means that the majority
of interactions with a PollEvented can happen with no coordination.

However, implementing this change re-raises the question of how to
read and write from separate tasks. AsyncRead::split in tokio-io offers
one solution. But that introduces coordination that isn't required in all
cases.

Specifically, mio::TcpStream is fully Sync. A "read" and a "write"
handle should require zero additional coordination. The PollEvented
read and write state can be split into separate fields.

A complete answer to this problem can be punted and split from
tokio-io provides an escape hatch.

Depends on #35.

[aturon]

I believe the original goal here was to surface the thread safety provided by the underlying OS primitives. But it's true that it globally imposes some coordination for us, and in practice it's essentially always a bug to race on these operations, so requiring that you've mediated &mut access seems not unreasonable. I think I'm OK with the change.

To be clear, when you talk about punting the problem (and splitting the read/write state), is the idea that we could offer a split method within PollEvented in the future?

This is one where I'd like input from @alexcrichton as well.

[cramertj]

Generally speaking, it is unsafe to access to perform asynchronous
operations using &self. Taking &self allows usage from a Sync
context, which has unexpected results.

Can you say more? I don't understand why it is unsafe to have two writers concurrently accessing the same UdpSocket, for example. Is there something in particular about async IO that makes this wrong?

Edit; oh, I see, you just mean that reads and writes intermixing is probably wrong, not that it's "unsafe" as in memory unsafety.

[aturon]

@carllerche on further reflection, I'd like to have a bit more confidence that we can allow you to split up the read/write halves without imposing any coordination. Can we spike that out?

[carllerche]

@aturon there is an entire gradient of safety with regards to splitting the read / write halves. The option that has the least overhead involves exposing some unsafe APIs and saying "you need to ensure to call the read & write halves in a coordinated way.

[carllerche]

@aturon I guess, the question is, what do you see the goal of the spike being?

a) To see if it is technically possible.
b) To explore APIs to potentially provide.

[carllerche]

@cramertj Yes, I mean unsafe from a usage P.O.V. (trampling the task being registered to receive notifications).

[aturon]

@carllerche I'm confused about your use of "unsafe" terminology here. Can you expand on that?

[carllerche]

Context

At a high level, PollEvented lets you read and write to an underlying I/O resource. It represents read related fns (read) as an async operation and write related fns (write, flush) as an async operation.

By async operation, I am referring to the contract described by Future::poll; i.e., if the operation is not ready to complete, NotReady is returned and the task from which the operation was issued is notified once the operation transitions to "ready to complete". To implement this behavior, PollEvented tracks two separate task values, one for "read" operations and one for "write" operations.

If a PollEvented operation is attempted and NotReady is returned, the current task is stored. The PollEvented may then be moved to a new task and the operation attempted again while still in the NotReady state. In this case, the original task value is replaced by the new task value as PollEvented will only notify the last task that attempted the operation.

If a PollEvented is used from a sync context, this implies that there are more than one tasks that are concurrently operating on the PollEvented. In order for this concurrent interaction be correct from a futures-rs task model perspective, there only may be two concurrent tasks operating on a single PollEvented value. One of these tasks must restrict itself to read operations and the other task must restrict itself to write based operations.

Goal

Since a PollEvented manages both read and write as separate operations, it is desirable to be able to perform these two operations independently on different tasks.

When the underlying I/O resource is Sync out of the box (mio::TcpStream), it is desirable to reduce the amount of coordination required.

A "hard to misuse" API should be provided to the end user.

Problem

By taking &self, PollEvented enables a single value to be used concurrently across multiple tasks. However, this API is not "hard to misuse". An end user could easily violate the requirement that there are only two tasks and each task performs a distinct operation on the PollEvented

On top of that, the implementation of PollEvented with &self requires a synchronization cost even when there is none required (some additional atomic ops).

This PR

As of now, this PR switched PollEvented to take &mut self instead of &self. This ensures that the value is not used concurrently from multiple threads (at least not without wrapping w/ a Mutex which at least provides additional friction before letting you do something bad).

The issue now is that there is no "low coordination" way to perform the read and write operations on separate tasks with a single PollEvented value. The options are basically to wrap it in a Mutex, but this adds overhead that should not be necessary when using a mio::TcpStream which is inherently Sync.

Where to go now.

Well, the real question is, how should a TcpStream be used concurrently from two different tasks. In order to provide a "hard to misuse" API, there needs to be some "split" operation (splitting the single value into a read half and a write half).

Since the goal is to leverage the inherent Sync nature of TcpStream, one option would be to add a sync_split (naming?) function directly on TcpStream that returns (tcp::ReadHalf, tcp::WriteHalf). This implementation would require an internal Arc to store the TcpStream because otherwise there is no way to know when to close it.

The last question that remains is, how does one get down to the lowest level and use TcpStream from two tasks without paying the cost of the Arc. While important, I would suggest that this is a very advanced requirement.

One option to deal with this would be to bump down a level and provide the necessary building blocks for users to achieve the desired behavior. For example, instead of having PollEvented be the building block, expose what IoToken is in this branch (though probably rename it to Registration). At that point, the advanced user could call tokio::TcpStream::into_parts() -> (mio::TcpStream, tokio::Registration) and then use those two building blocks to access the mio::TcpStream across tasks at the lowest possible overhead.

This would be an advanced API as there would be no protection against accidentally reading from the TcpStream concurrently.

[aturon]

@carllerche Ok, thanks -- that all matches my understanding. But just to be 100% clear, you use of the term "unsafe" before was loose; there's no risk of memory safety violations here, "just" bugs. The splitting strategy sounds fine to me.

[carllerche]

Yes, no memory unsafety. Only misuse bugs.

[alexcrichton]

Hm I don't think I'd personally be in favor of this change. 100% of the time in the past we've regretted adding anything on top of the OS that wasn't already there, such as the "implicit synchronization" here of using &mut T vs &T.

I think it's definitely possible to misuse the shared reference impls, but I don't think the solution is to just make all those use cases impossible without extra synchronization? We could perhaps explore something like dynamic detection of misuse of tasks to be enabled in debug mode or something like that.

[carllerche]

@alexcrichton Could you expand more on your reservations? "100% of the time in the past we've regretted adding anything on top of the OS that wasn't already there" doesn't really address the proposal.

I don't think the solution is to just make all those use cases impossible without extra synchronization

I am not proposing to make it impossible. In fact, I personally care to be able to do it.

What I am trying to achieve is:

1. Avoid needless additional synchronization
2. Provide "hard to misuse" APIs
3. Provide lower level APIs (potentially unsafe in the memory safety sense) that are fully flexible.

Point 3) could be addressed by something like:

impl<T> PollEvented<T> {
    /// Requires that the caller guarantees that calls to this function are coordinated.
    unsafe fn uncoordinated_reader(&self) -> UncoordinatedReader
    where for<'a> &'a T: io::Read
    {
        // ...
    }
}

To be clear, the current PR would need to change. Specifically, the readiness field on PollEvented would need to be split into two fields. One for read readiness and one for write readiness.

We could perhaps explore something like dynamic detection of misuse of tasks to be enabled in debug mode or something like that.

This would be interesting to explore regardless, but making it hard to make the mistake in the first place is ideal.

[aturon]

@alexcrichton is out sick right now, but he and I discussed this some offline, and while he still has hesitations, I think we can go forward with it.

[carllerche]

@aturon There is no urgency to get this merged. We can wait until next week.

[cramertj]

unsafe fn uncoordinated_reader

Just to clarify, the unsafe here is because you need to make sure that accesses to the inner readiness value don't data race, and you intend to wrap readiness in UnsafeCell?

[carllerche]

@cramertj It is unsafe because the caller needs to ensure to coordinate calls to uncoordinated_reader. It would use an UnsafeCell (or some other strategy) for managing state.

[cramertj]

@carllerche

because the caller needs to ensure to coordinate calls

Right, I'm trying to clarify why failing to coordinate calls would cause memory unsafety or data races. If there's not an underlying justification in terms of memory unsafety or data races, I don't think that unsafe should be used to lint against uses of this method.

[carllerche]

Because it would be possible to mutate the same data from across multiple threads (it is unsafe in the rust sense).