Signal safety

[3Hren]

Some low- and high-level functions are signal unsafe. That means if Read or Write given returns an Err with EINTR there are no way to continue decoding/encoding properly.

• Investigate which functions and traits are signal unsafe by marking it in a documentation.
• Implement signal-safe alternatives if possible.

[bombela]

The documentation of Read::read says:

If this function encounters any form of I/O or other error, an error variant will be returned. If an error is returned then it must be guaranteed that no bytes were read.

And the documentation of Read::read_to_end says:

If this function encounters an error of the kind ErrorKind::Interrupted then the error is ignored and the operation will continue.

Finally the documentation of BufRead::read_until says:

This function will ignore all instances of ErrorKind::Interrupted and will otherwise return any errors returned by fill_buf.

But also adds:

If an I/O error is encountered then all bytes read so far will be present in buf and its length will have been adjusted appropriately.

In light of the above, I see two solutions to the EINTR problem:

1. Return EINTR on first operation, ignore further EINTR:
   • If the first operation in the decoding/encoding function returns ErrorKind::Interrupted (EINTR) then return EINTR.
   • Any further EINTR after a successful read/write are ignored (ie: the operation is retried until it doesn't EINTR).
2. Forward EINTR as soon as it happens.
   • This requires buffering and keeping some states about every operations.
   • Any {de,en}coding function can be resumed by calling it again until there is no more EINTR.

First solution

With the first solution, any operation either aborts fast, or succeed fully, no matter how many EINTR are encountered. There is no 'in-between' state on the Read/Write objects, the msgpack data is never half-consumed. The current code can be updated trivially to follow this convention.

If you call read_value on a huge msgpack stream, you get nothing or everything. However any user of the library is free to implement his own state-machine to read the msgpack stream in pieces, but rmp guaranties that any operation requested, recursive or not, is either aborted quickly or successful.

• pros: easy to implement, stateless.
• cons: hides some EINTR, user must manage state himself for finer control.

Second solution

In the second solution, the decoder is now state-full. It has the advantage of returning quickly on EINTR, at any level of recursion, instead of hiding them. The underlying Read/Write object can be consumed/written to partially. An rmp operation must be re-started until it succeeds to get the Read/Write object to the next sane state.

• pros: do not hide EINTR, returns them all.
• cons: must maintain a stack, state-full, probably some overhead with temporary buffers/values (need to represent half-decoded/encoded values)

So looking at the two solutions + the documentation exert above, and the first solution look most reasonable to me.

Until you s/EINTR/EAGAIN/ and suddenly, the trade-off is not as clear (hiding EGAIN will basically block any asynchronous loop...).

[bombela]

For what its worth, note that rustc_serialize/json parser is state-full: http://doc.rust-lang.org/num/rustc_serialize/json/struct.Parser.html

[3Hren]

Wow, great analysis! Thank you!

I'd personally prefer the first solution, but only for a set of low-level functions (like reading/writing primitives, maybe bin/str), which doesn't read/write much data.

Potential huge-data reading/writing function (like read_value and Decoder/Encoder probably will be statefull.

I'll start working with more detailed plan this Sunday. Of course with complete testing suite attached.

[3Hren]

The problem is more wide than I expected before: the current library just works improperly on any I/O errors without external control.

How about constraining stateless functions (which knows the required bytearray size >= 2) to accept BufRead except of Read?
This change allows us to save all bytes obtained between attempts until fully read, regardless of any I/O errors occurred (not just EINTR). In other words we just move the required "state" outside the function into the parameter.

For example:

/// Requires 5 bytes to be read.
fn read_u32<R: BufRead>(rd: &mut R) -> Result<u32> {
    let buf = try!(rd.fill_buf());

    if buf.len() < 5 {
        return Err(Error::InsufficientBytes);
    }

    let marker = ...;
    // Check marker.

    let val = ...;

    // If everything is ok - consume bytes.
    rd.consume(5);

    Ok(val)
}

On the other hand this wrappers looks like boilerplates...

[bombela]

This doesn't compose: reading an array requires to read the size + size elements. You could successfully read the size and a couple elements before encountering any error. Then, how do you restart where you left?

Note the API of rustc_serialize doesn't work with interruptions. Theirs solution is to parse the json stream into some json value (like your Value type) keeping an explicit stack to handle interruptions in the parser. And then the json Value implements Decodable to decode it into a Rust type.

I do like your approach of decoding directly from the stream of data, this does feel more efficient. Since I do not require interruption handling for what I am working on: I always have a buffer containing the whole msgpack in hand; Read will never be interrupted. This is where I like that I can decode directly into the destination type instead of passing trough the overhead of an intermediate Value.

[3Hren]

I restart from the beginning. Until explicitly consumed ReadBuf will return bytes cached.

[3Hren]

It appears that all functions silently handle interruptions by restarting its underlying operations in a proper way.

Also I've decided not to save data read on any other I/O error, leaving this action on the user's responsibility.

[bombela]

This is not what is documented. And not what happens in practice. On UNIX a read on a Socket goes trough the FileDesc read: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/fd.rs#L40

Which in turns call the libc read (http://linux.die.net/man/2/read). EINTR and EAGAIN are explained with plenty of details in http://linux.die.net/man/7/signal:

read(2), readv(2), write(2), writev(2), and ioctl(2) calls on "slow" devices. A "slow" device is one where the I/O call may block for an indefinite time, for example, a terminal, pipe, or socket. (A disk is not a slow device according to this definition.) If an I/O call on a slow device has already transferred some data by the time it is interrupted by a signal handler, then the call will return a success status (normally, the number of bytes transferred).

And http://linux.die.net/man/7/socket:

It is possible to do nonblocking I/O on sockets by setting the O_NONBLOCK flag on a socket file descriptor using fcntl(2). Then all operations that would block will (usually) return with EAGAIN (operation should be retried later); connect(2) will return EINPROGRESS error. The user can then wait for various events via poll(2) or select(2).

Leaving this action to the user's responsibility is perfectly fine, but then rmp should probably not work on Read and Write trait since it would lead somebody to believe a socket can be used directly, which is not the case.

Obviously, Read and Write on slice/vec should not return interrupted/tryagain errors, so this works only in this specific case.

[3Hren]

I mean all rmp's function silently handle interruptions.

For non-blocking sockets, usually there are wrappers, which handle EAGAIN, EWOULDBLOCK and EINPROGRESS internally. If someone intentionally want to use manually set raw non-blocking socket as Read/Write, then there is a notice about possible data loss. I just can't handle all possible errors without data buffering.

Or do you see another solution?