Skip to content

Commit

Permalink
Calculate the actual mask to be removed in the eventloop before aeApi…
Browse files Browse the repository at this point in the history
…DelEvent (valkey-io#725)

for kqueue:  
EV_DELETE fails if the specified fd is not associated with the kqfd. If
EVFILT_WRITE is associated but EVFILT_READ is not, then calling
aeApiDelEvent with mask = -1 or `(AE_READABLE|AE_WRITABLE)` will
cause the kevent() to fail with errno = 2(No such file or directory) and
EVFILT_WRITE not dissociated. So we need to calculate the actual mask
to be removed, instead of passing in whatever user provides.

for evport:  
The comment clearly states that aeApiDelEvent "rely on the fact that our
caller has already updated the mask in the eventLoop".

for epoll:  
There's no need to calculate the "actual mask" twice, once in
`aeDeleteFileEvent` and another in `aeApiDelEvent`, let's just use the
mask recorded in the eventLoop.

Fixes valkey-io#715 

Signed-off-by: wkgcass <[email protected]>
Co-authored-by: Andy Pan <[email protected]>
Co-authored-by: Binbin <[email protected]>
  • Loading branch information
3 people authored Jul 9, 2024
1 parent bbfd041 commit 548b4e0
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 3 deletions.
13 changes: 12 additions & 1 deletion src/ae.c
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ void aeDeleteFileEvent(aeEventLoop *eventLoop, int fd, int mask) {
* is removed. */
if (mask & AE_WRITABLE) mask |= AE_BARRIER;

aeApiDelEvent(eventLoop, fd, mask);
/* Only remove attached events */
mask = mask & fe->mask;

fe->mask = fe->mask & (~mask);
if (fd == eventLoop->maxfd && fe->mask == AE_NONE) {
/* Update the max fd */
Expand All @@ -193,6 +195,15 @@ void aeDeleteFileEvent(aeEventLoop *eventLoop, int fd, int mask) {
if (eventLoop->events[j].mask != AE_NONE) break;
eventLoop->maxfd = j;
}

/* Check whether there are events to be removed.
* Note: user may remove the AE_BARRIER without
* touching the actual events. */
if (mask & (AE_READABLE | AE_WRITABLE)) {
/* Must be invoked after the eventLoop mask is modified,
* which is required by evport and epoll */
aeApiDelEvent(eventLoop, fd, mask);
}
}

void *aeGetFileClientData(aeEventLoop *eventLoop, int fd) {
Expand Down
6 changes: 4 additions & 2 deletions src/ae_epoll.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,12 @@ static int aeApiAddEvent(aeEventLoop *eventLoop, int fd, int mask) {
return 0;
}

static void aeApiDelEvent(aeEventLoop *eventLoop, int fd, int delmask) {
static void aeApiDelEvent(aeEventLoop *eventLoop, int fd, int mask) {
aeApiState *state = eventLoop->apidata;
struct epoll_event ee = {0}; /* avoid valgrind warning */
int mask = eventLoop->events[fd].mask & (~delmask);

/* We rely on the fact that our caller has already updated the mask in the eventLoop. */
mask = eventLoop->events[fd].mask;

ee.events = 0;
if (mask & AE_READABLE) ee.events |= EPOLLIN;
Expand Down

0 comments on commit 548b4e0

Please sign in to comment.