file read EOF condition

[BurningEnlightenment]

I am unsure how to portably detect an EOF condition for a file_handle in cases where I sequentially consume the whole file, e.g. for computing its hash or copying its content. My usual approach is to open the file and just loop until the read indicates the EOF condition (e.g. pread() returning 0 or ReadFile() failing with ERROR_HANDLE_EOF).

P1031r2 states in its io_handle::read() specification, and I quote:

At least the first byte of the first of the non-zero sized input buffers will be filled if the function
returns successfully.

So I would expect that a read() call at the file end either (waits until new data is appended or the deadline expires) or (fails with an error which can be portably distinguished). However, the specification also states:

It is implementation defined if a read exceeds any current maximum extent of the storage
referenced by the handle. It may clamp buffers returned to that maximum extent (i.e. by
partially filling the last fillable buffer, and marking all remaining buffers as being unfilled), or
fill with garbage instead, or clamp to a different maximum extent, or any other behaviour.

Which I assume to loosen the specification to implementation defined behavior in my case. In order to stay portable I would need to check the maximum file extent beforehand which leads me into TOCTOU land.
The current reference implementation just passes through the behavior described in the first paragraph.
All in all it seems to me either that the case is underspecified for plain files or that I missed something. Maybe specifying the posix pread() behavior and aligning the windows implementation is a reasonable solution?

[ned14]

You are completely right that for io_handle::read(), one can no longer detect EOF because we now tolerate partial buffer fills more than before R2. This is because an io_handle implementation could be a never terminating stream of data, a polling implementation, or anything else.

For file_handle::read() you are absolutely guaranteed that a partial or empty buffer fill equals EOF, at least for the EOF at the instantaneous moment of that read (the file may be concurrently being extended). That's not documented in R2 yet, due to lack of my free time.

For async_file_handle::read() we currently have a race condition in how partial buffer fills are implemented, and that's tracked by #20. But R2's io_handle::read() wording has been improved to handle async_file_handle::read() returning less buffers than were actually filled in order to fix that bug.

Does this make sense?

[BurningEnlightenment]

You are completely right that for io_handle::read(), one can no longer detect EOF because we now tolerate partial buffer fills more than before R2. This is because an io_handle implementation could be a never terminating stream of data, a polling implementation, or anything else.

That was my interpretation, too. I only quoted the io_handle::read() spec, because currently [llfio.io.file_handle] reads Todo and the implementation of file_handle::read() is the same as io_handle::read().

For file_handle::read() you are absolutely guaranteed that a partial or empty buffer fill equals EOF, at least for the EOF at the instantaneous moment of that read.

This is the part I wanted to be clarified and I think it is the only sane way to specify it. Which brings me to my actual point: The current file_handle::read() windows implementation does not adhere to this contract, because it doesn't "catch" ERROR_HANDLE_EOF:

llfio/include/llfio/v2.0/detail/impl/windows/io_handle.ipp

Lines 91 to 95 in 14d5c7f

	if(!syscall(nativeh.h, req.data(), static_cast<DWORD>(req.size()), &transferred, &ol) && ERROR_IO_PENDING != GetLastError())
	{
	return win32_error();
	}
	reqs.offset += req.size();

which in turn forces me to write an ugly wrapper with a win32 special case, because there is no portable eof error_condition:

inline auto readx(llfio::file_handle &src, std::size_t offset, rw_dynblob buffer) noexcept
    -> result<rw_dynblob>
{
    llfio::io_handle::buffer_type raw_buffer{buffer.data(), buffer.size()};
    if (auto readrx = src.read({{&raw_buffer, 1}, offset}))
    {
        return rw_dynblob{readrx.assume_value()[0]};
    }
    else
    {
        auto ec = make_error_code(readrx.assume_error());
#if defined BOOST_OS_WINDOWS_AVAILABLE
        if (ec == std::error_code{38, std::system_category()}) // ERROR_HANDLE_EOF
        {
            return rw_dynblob{};
        }
#endif
        return failure(ec);
    }
}

something I would rather avoid if possible.
So if I sum it up correctly, the solution will be to override read() in file_handle and turn ERROR_HANDLE_EOF into an empty result buffer, correct? Shall I go ahead and implement that?

Does this make sense?

Very much so, I think. Thanks for providing the insight.

[ned14]

This is the part I wanted to be clarified and I think it is the only sane way to specify it. Which brings me to my actual point: The current file_handle::read() windows implementation does not adhere to this contract, because it doesn't "catch" ERROR_HANDLE_EOF:

Surely that error only occurs if the read offset is wholly beyond the current maximum extent?

If so, that exactly matches POSIX on this. Same behaviour. And totally system-introduced. LLFIO tries to avoid fiddling when we can pass through, unless there is an extremely good reason not to (e.g. that race in Win32 CreateFile() which LLFIO works around).

there is no portable eof error_condition

Yeah, you're kinda caught there. POSIX returns invalid parameter for when you try to wholly read past EOF. Microsoft do not map ERROR_HANDLE_EOF to EINVAL, as you can see at https://github.com/ned14/status-code/blob/master/include/detail/win32_code_to_generic_code.ipp. So this is actually a bug in MSVC's win32 error code mapping, and not inside my control.

So if I sum it up correctly, the solution will be to override read() in file_handle and turn ERROR_HANDLE_EOF into an empty result buffer, correct? Shall I go ahead and implement that?

You are totally free to subclass LLFIO classes and override anything you like. It was designed to allow that, so one can solve local weirdnesses like yours. But a more proper solution would be to subclass std::system_category, and fix the mapping there.

But fixing it for everybody globally, that's not wise. Microsoft like to change their Win32 error code mappings, as do all the standard library implementations. So LLFIO makes no attempt to fix the many mismappings in all the standard libraries. I do control status code however, and I am happy to fix it there. Raise a bug on https://github.com/ned14/status-code/issues if you'd like that.

[BurningEnlightenment]

Surely that error only occurs if the read offset is wholly beyond the current maximum extent?

It also occurs for reads with offset = maximum extent, e.g. when I consume the file in 128kiB chunks and the file extent is a multiple of the chunk size. This means that the attached example prints failed with code 38; Reached the end of the file. But I would expect it to print successfully read 0B as the posix pread() spec states:

No data transfer shall occur past the current end-of-file. If the starting position is at or after the end-of-file, 0 shall be returned.

LLFIO_V2_NAMESPACE::result<void> repro()
{
    namespace llfio = LLFIO_V2_NAMESPACE;
    auto name = llfio::utils::random_string(8);
    OUTCOME_TRY(
        o, llfio::file({}, name, llfio::handle::mode::write, llfio::handle::creation::if_needed));

    std::vector<std::byte> zeroes(128 * 1024, std::byte{});
    OUTCOME_TRY(o.write(0, {{zeroes.data(), zeroes.size()}}));

    OUTCOME_TRY(o.close());

    OUTCOME_TRY(i, llfio::file({}, name));

    llfio::file_handle::extent_type offset{0};
    OUTCOME_TRY(num_read, i.read(0, {{zeroes.data(), zeroes.size()}}));
    offset += num_read;
    if (auto rx = i.read(offset, {{zeroes.data(), zeroes.size()}}))
    {
        std::cout << "successfully read " << rx.assume_value() << "B\n";
    }
    else
    {
        auto ec = std::move(rx).assume_error();
        std::cout << "failed with code " << ec.value() << "; " << ec.message() << "\n";
    }

    OUTCOME_TRY(i.unlink());
    return llfio::success();
}

[ned14]

I did some more research into this, and what happens if you read at or beyond EOF varies between systems:

• Windows: error >= ERROR_HANDLE_EOF
• Linux & BSD: EINVAL < 0, otherwise succeed with 0 bytes read
• HPUX & SunOS: EOVERFLOW >= EOF

So Windows isn't being weird here. It chose to track SunOS, which makes sense given its vintage.

I'm minded to leave in the platform-specific behaviour, but add in a note saying you're into implementation defined behaviour >= EOF. Is this okay with you?

[BurningEnlightenment]

Well, I'm not particularly happy about specifying it as implementation defined, but I can deal with it. In this case I would really appreciate the ability to suppress the EOF errors from the llfio log, because currently a myriad of EOF errors are cluttering my logs which is quite unpleasant to work with.

However, I would argue that all platforms provide a way to cleanly detect the EOF condition, therefore there ought to be a portable interface for it in the platform abstraction layer. Maybe we could add an API to io_result? I know that outcome reserves 16Bits in its internal result state for user defined purposes and we could use one of these as an EOF indicator. So the implementation would do the platform specific EOF detection, but still just pass through any errors reported by the OS.

[ned14]

Firstly, that's a great idea about filtering certain errors from the log. I too have suffered from too much log detail. I'll honestly admit that it hadn't occurred to me to simply add a filter to the log. Thank you.

As you have correctly observed, LLFIO avoids mentioning EOF. This is because it's racy. One moment you might experience EOF, a microsecond later you might not.

You may have noticed that i/o gets returned using io_result rather than result. This was to enable adding member functions to query an io_result for certain bespoke things. Right not, there is only .bytes_transferred(). I think it reasonable to add an .is_incomplete(), or something along those lines, which can say if the result filled less data than what was requested. I believe it should be free of cost to set such a bit in the result.

I can't give an ETA on this work, unfortunately. It's likely after the Cologne meeting, where I hope to get the first tranche of official WG21 feedback on this library. Also, I'm currently working on gutting and replace the signal handling infrastructure in LLFIO based on WG21 feedback, which has the side effect of a major refactor of how completion handlers are stored and invoked. So, I am technically working on LLFIO, just indirectly for now. This choice of priority is because there was surprising warmth for a complete replacement for signal handling on all three of WG21, WG14 and POSIX. Literally everybody was "bring it on already", which surprised me.

[BurningEnlightenment]

You may have noticed that i/o gets returned using io_result rather than result. This was to enable adding member functions to query an io_result for certain bespoke things. Right not, there is only .bytes_transferred(). I think it reasonable to add an .is_incomplete(), or something along those lines, which can say if the result filled less data than what was requested. I believe it should be free of cost to set such a bit in the result.

Yeah, that would be perfectly fine with me.

I can't give an ETA on this work, unfortunately. It's likely after the Cologne meeting, where I hope to get the first tranche of official WG21 feedback on this library. Also, I'm currently working on gutting and replace the signal handling infrastructure in LLFIO based on WG21 feedback, which has the side effect of a major refactor of how completion handlers are stored and invoked. So, I am technically working on LLFIO, just indirectly for now. This choice of priority is because there was surprising warmth for a complete replacement for signal handling on all three of WG21, WG14 and POSIX. Literally everybody was "bring it on already", which surprised me.

I'm not in a hurry and just using my current patch set will suffice for some time. Signal handlers are a sore point which I always avoid to touch if possible; very kind of you to improve that situation.