Bug 12987 - [tevent] FD errors are not propagated when waiting for writeability
Summary: [tevent] FD errors are not propagated when waiting for writeability
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-25 17:48 UTC by Robbie Harwood
Modified: 2019-02-02 23:10 UTC (History)
3 users (show)

See Also:


Attachments
tevent.c (1.39 KB, text/x-csrc)
2017-08-25 17:48 UTC, Robbie Harwood
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robbie Harwood 2017-08-25 17:48:53 UTC
Created attachment 13506 [details]
tevent.c

Apologies for wrong product/component; I don't see a place to file tevent bugs directly.

tevent does not fire the write callback when a waiting fd (tevent_add_fd) enters an error state if it is only waiting for the fd to become writeable.  However, it does fire if the callback is registered for the fd becoming readable.

With the attached program as written, this looks like:

    rharwood@seton:~$ ./tevent 
    [tevent_loop_once]
    [tevent_loop_once]
    [timer_cb]; error!
    rharwood@seton:~$ echo $?
    255

However, if line 46 is changed to TEVENT_FD_READ, it instead does this:

    rharwood@seton:~$ ./tevent 
    [tevent_loop_once]
    blue skied an' clear
    rharwood@seton:~$ echo $?
    0

This behavior was uncovered by libverto's test suite, since the interface libverto presents expects fd errors to be passed up in both read and write calls.  The corresponding tracker bug is https://github.com/latchset/libverto/issues/16

Simo says this has to do with how multiplexing of epoll events happens in tevent.  We're not sure what the implications of this change would be on other consumers of tevent.  One possibility could be to add a flag (TEVENT_FD_ERROR?) to control this behavior.

Thanks!
Comment 1 Jeremy Allison 2017-08-25 17:56:22 UTC
This is by design currently. See dbcdf3e39c359241b743a9455ae695e14a30caa9. All select for writing should check is that a write will not block.

If you want to detect errors you should check for reads.
Comment 2 Jeremy Allison 2017-08-25 18:03:17 UTC
The issue is select(). As tevent can be built on top of select as well as epoll/ports/poll then it has to export the lowest common denominator behavior on all the systems that support these interfaces.
Comment 3 Jeremy Allison 2017-08-25 18:05:40 UTC
Does libvertio test an event library based on top of select() ? My guess is that your https://github.com/latchset/libverto/blob/master/tests/write.c code would fail on such a library.
Comment 4 Jeremy Allison 2017-08-25 18:32:33 UTC
Looking more closely at:

https://github.com/latchset/libverto/blob/master/tests/write.c

it doesn't look like a good test to me. It's doing a 5 byte write of the "hello" string, then adding a read() callback get called and read the data, after which it close()'s the read end of the pipe.

This test code is assuming that a newly added read() callback will get priority over an existing write() callback. That's not necessarily true.

What happens if you're using an underlying event library that prioritizes write events over reads ?

In that case your write() callback will be repeatedly called, until you finally hit the pipe buffer limit and then the code will block on the write (as you're single threaded and have no reader).

You need to think about tests more carefully than this. You're getting away with it by sheer dumb luck. Once the write callback has fired you should remove the writable flag at the same time as adding the read callback request.

Check the test code inside lib/tevent/testsuite.c for some more complicated tests that deal with differing pipe buffer sizes and different pipe behaviors on different systems (this code also runs on FreeBSD and Solaris too).
Comment 5 Simo Sorce 2017-08-25 18:39:16 UTC
The test suite may not be the best code, but it is testing that a program that is waiting for a pipe to become writable gets an error if that pipe is closed on the other side.

At the momoent what happens if a process is not reading and eventually dies is that we never exit this state as far as I can see, unless we also have a timeout event on the fd.

Is this intentional ?
Or do we rely on the fact that we should always have a read callback because nothing will trigger it in this situation except to report an error condition ?

It still seem that we could very easily report an error if the only event we have for a specific fd is a write event, not clear why we do not do that, what's the harm ?

(As for the select() quetion, who even still uses select ? verto certainly doesn't unless it is used in one of the underlying event libraries by accident)
Comment 6 Jeremy Allison 2017-08-25 19:28:15 UTC
Another issue with https://github.com/latchset/libverto/blob/master/tests/write.c is that in the read callback you're calling 

close(fd) on:

int fd = verto_get_fd(ev); 

Without telling the underlying event library about it. That usually doesn't end well (at least in the tevent_select backend this will cause the loop to return with -1).
Comment 7 Jeremy Allison 2017-08-25 19:35:19 UTC
(In reply to Simo Sorce from comment #5)

You need to select for readability to detect an error. If you aren't intending anything to be read, the fact that it becomes readable means that (a) someone wrote to it and they weren't supposed to :-), or (b) someone closed the opposite side. Trying to detect errors by checking for writability is non-portable (yes I know you only care about Linux, but tevent has to care for more than that) and will have mysterious failure modes in complex code later. Might work fine on a simple pipe() test, but not so much on network communications.

The clue to all of this is the fact that your test program does a close() on the fd that's supposed to be handled by the underlying event library and doesn't tell it about it. That should tell you that you're doing something wrong. At that point your program should be cleaning up.
Comment 8 Robbie Harwood 2017-08-25 19:42:44 UTC
(In reply to Jeremy Allison from comment #6)
The callback is unregistered immediately after that; it shouldn't matter I don't think?
Comment 9 Jeremy Allison 2017-08-25 19:51:26 UTC
So what your test is testing that in the specific case where a pipe-created file descriptor is passed into the event library, that closing the read-side will cause writability on the write-side fd. That's a very specific test for a specific kind of file descriptor passed into the event library. Such a test could easily fail on a network connected socket I think, depending on the state of the network.

Do you want to encourage users of libverto to depend on fd-specific behaviour when they 'know' they're passing a pipe-created fd in ? Do you want them to have to write different code when they 'know' they're dealing with network connected file descriptors ? I'd argue this is not a good design decision to make for libverto users.

Any complex code is going to have to handle only-detect-close-on-readability semantics for all non-trivial communication anyway, so why not make them bite the bullet in design decisions rather than once they've prototyped trivial test code ?
Comment 10 Jeremy Allison 2017-08-25 19:54:56 UTC
(In reply to Robbie Harwood from comment #8)

Is it unregistered ? Is verto_add_io() an API that overwrites the current VERTO_EV_FLAG_IO_XXX settings ? It doesn't seem so at a quick glance (but I haven't gone into the code so carefully). If it overwrites all other io callbacks, why is it called _add_ ?

I don't see anywhere where you unregister the read callback before of after the close(). Am I missing a call ?
Comment 11 Jeremy Allison 2017-08-25 19:56:13 UTC
Or is verto_add_io() adding only a oneshot callback ?
Comment 12 Jeremy Allison 2017-08-25 20:16:54 UTC
So thinking about this some more, if you *really* must have these semantics in the tevent backend I can see how to implement it.

When adding a write callback you need to check the flags on the underlying file descriptor, and if it doesn't already have a read callback then add one. As you know nothing should be reading here, then if it fires you can treat it as an error of 'remote end closed'.

You'll need to keep a list of fd's that tevent currently knows about and what flags you have set on them so you can determine when the underlying fd changes from write-only to read-write or read-only and adjust the callbacks accordingly.

This is just another reason you shouldn't get in the habit of pulling the file-descriptor rug out from under the underlying event library by doing close() on it without telling it :-).

If you do this then at least the tevent backend for libvirto will cope correctly with remote end closing on a write-only socket over the network, although I'm not sure if the other backends will cope as well :-).

I don't think this is an underlying tevent bug though. Maybe metze might also comment.
Comment 13 Simo Sorce 2017-08-25 20:36:06 UTC
I guess the reason why we opened this bug is that the 3 other event libraries seem to support this natively, so we were very surprised to see that tevent *actively* discard errors that come from epol and ignores them.

We can definitely work around this behavior, but it is surprising for someone that knows epoll and other event libraries.
Comment 14 Jeremy Allison 2017-08-25 21:13:10 UTC
I think this comes from the historical nature of tevent. Initially the only backends were select() and poll() (I think epoll was added later), and some systems select() does not detect this error, so the poll() (and epoll() code paths were modified to expose exactly the same behavior as the select() backend).

Now select() is deprecated and might possibly be removed, we could possibly expose this, although this is a behavior change so probably only on a major number release.

Having said that, I don't think this behavior is a good thing to expose for users of libverto, as it exposes differences between pipe() and network socket() file descriptors to the library users.

Do you really want to do that ?
Comment 15 Stefan Metzmacher 2017-08-27 12:18:59 UTC
(In reply to Robbie Harwood from comment #0)

I think adding TEVENT_FD_ERROR and removing of the select backend would be
the way to go.

I'll were I have my TEVENT_FD_ERROR I wrote a few years ago.
Comment 16 Andreas Schneider 2019-02-02 23:10:02 UTC
Any news about that?