Race condition in "set" can lead to a bad state

[DanielShaulov]

There is an assumption that at any time, there is either 1 or 0 bytes in the pipe.
This assumption breaks if 2 threads call set, each checks the flags, sees that it's false, sets it to true and writes a byte to the pipe. After that - if we call clear, it will only read 1 of the bytes, so the flag will be false but the fd will still be readable, a wait will return immediately without actually waiting for a set in that case.

[palaviv]

I think the above situation can't happen because of the python GIL. The first set will not release the GIL until it sets the internal flag.

[DanielShaulov]

The GIL only promises that each bytecode is an atomic operation, but you have a read and then a write, and there is nothing stopping the GIL from being released between the two operations.

Also the GIL is an implementation detail that you should not rely upon.

[larslue]

We are relying on eventfd for various notifications. Since a while we are getting this AssertionError every once in a while in seemingly random moments. It is reproducible and is likely the said race condition between calling BaseEventFD.clear and BaseEventFD.set.

This is the stack trace:

Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/usr/lib/python2.7/threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "/opt/ros/melodic/lib/python2.7/dist-packages/catkin_django/notifications.py", line 151, in _work
    self._event_add_sub.clear()
  File "/usr/lib/python2.7/dist-packages/eventfd/_eventfd.py", line 51, in clear
    assert self._read(len(self._DATA)) == self._DATA
AssertionError

I was able to fix it by locking the access to the methods:

import threading

from eventfd import EventFD

class LockedEventFD(EventFD):
    def __init__(self):
        super(LockedEventFD, self).__init__()
        self._lock = threading.Lock()
    def clear(self):
        with self._lock:
            return super(LockedEventFD, self).clear()
    def set(self):
        with self._lock:
            return super(LockedEventFD, self).set()

[takluyver]

I think this comes about because self._flag is duplicating part of the information in the file descriptor, and they can get out of sync. One obvious solution is to use a lock, as in @larslue's example, so the check and modification happen together.

It's tempting to remove the duplication, and have is_set() poll the file descriptor (e.g. with a selector) to check if it's ready for read. I think this is simple with native eventfds, assuming that we never max out the 64-bit counter. For the pipe & socket fallbacks, you would also need to poll before writing so that .set() didn't block when the buffer filled up. I don't know how this would compare on performance, though.

[takluyver]

I had a go at fixing this in #10.