Thomasdezeeuw / gaea

Low-level library to build event driven applications, supporting lightweight non-blocking I/O.
https://docs.rs/gaea/
MIT License
16 stars 0 forks source link

Eventfd should be edge-triggered. #54

Closed cynecx closed 5 years ago

cynecx commented 5 years ago

https://github.com/Thomasdezeeuw/mio-st/blob/090c62073c3f8cf3a9fbfd969e02b437e7aa3c2d/src/sys/unix/awakener.rs#L30

I think this may be wrong. It should be edge-triggered instead. Here is why:

https://github.com/Thomasdezeeuw/mio-st/blob/090c62073c3f8cf3a9fbfd969e02b437e7aa3c2d/tests/awakener.rs#L48-L62

If you look at the test above, it seems to be checking that two different events are triggered subsequently. But this is not true. This simply works because the eventfd has been set level-triggered, meaning that each time epoll_wait is called, it will produce an event that data is available, because of the fact that we are not reading from the eventfd at-all!

This observation can be reproduced by simply commenting-out the lines 51-54. You will see that the test passes, because you are actually testing some other semantics and not the actual one here.

Solution: The eventfd should be registered edge-triggered.

Thomasdezeeuw commented 5 years ago

I think your right, however won't that mean that after every wake up you need to call drain? Will edge level notifications still wake up epoll instance?

Thomasdezeeuw commented 5 years ago

@cynecx seems you're right, I've created #55.

cynecx commented 5 years ago

You won’t need to call drain, an edge event handles this perfectly. The only downside is that the eventfd counter might overflow because each time we call write(evfd, &1u64, 8), we are incrementing an internal counter.

We could fix that by checking the return code of the write:

Thomasdezeeuw commented 5 years ago

@cynecx I've created another pr #56 that removes drain and does it automatically when writing fails.