[BUG] inconsistent behavior of kqueue aeApiDelEvent comparing to the code before #638

[wkgcass]

Describe the bug

If writable is registered into the kqueue and readable is not, aeDeleteFileEvent will fail if the mask is AE_READABLE | AE_WRITABLE or simply -1
Tested on macOS Ventura 13.6.7, x86_64.

To reproduce

1. only watch AE_WRITABLE events using aeCreateFileEvent
2. call aeDeleteFileEvent with mask = -1
3. the writable event still fires

Expected behavior

The writable event should not fire after calling aeDeleteFileEvent with mask = -1

Additional information

This is related to #638

The current ae_kqueue#aeApiDelEvent uses a single kevent to remove both readable and writable events with a single syscall,
but unlike EV_ADD, the EV_DELETE fails if the EVFILT_xxx is not previously registered into kqueue.

If EVFILT_READ is not registered, the kevent will fail and EVFILT_WRITE will not be deleted.

This behavior can be verified by the following simple code snippet:

#include <stdio.h>
#include <sys/event.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <errno.h>
#include <string.h>

int main(int argc, char** argv) {
    int kqfd = kqueue();
    printf("kqfd = %d\n", kqfd);

    int listenfd = socket(AF_INET, SOCK_STREAM, 0);
    printf("tcp listen fd = %d\n", listenfd);

    struct sockaddr_in bind_addr = {0};
    bind_addr.sin_family = AF_INET;
    bind_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    bind_addr.sin_port = htons(8000);

    bind(listenfd, (struct sockaddr*)&bind_addr, sizeof(bind_addr));
    listen(listenfd, 1);

    int sock = accept(listenfd, NULL, NULL);
    printf("accepted fd = %d\n", sock);

    struct kevent evs[2];
    EV_SET(&evs[0], sock, EVFILT_WRITE, EV_ADD, 0, 0, NULL);
    int err = kevent(kqfd, evs, 1, NULL, 0, NULL);
    printf("add EVFILT_WRITE result = %d\n", err);

    EV_SET(&evs[0], sock, EVFILT_READ, EV_DELETE, 0, 0, NULL);
    EV_SET(&evs[1], sock, EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
    err = kevent(kqfd, evs, 2, NULL, 0, NULL);
    printf("del EVFILT_READ and EVFILT_WRITE result = %d, errno=%d(%s)\n", err, errno, strerror(errno));

    int ret = kevent(kqfd, NULL, 0, evs, 2, NULL);
    printf("kq_poll returns %d\n", ret);
    for (int i = 0; i < ret; ++i) {
        struct kevent* e = &evs[i];
        int fd = e->ident;
        int filter = e->filter;

        printf(">>> fd=%d", fd);
        printf(", filter=");
        if (filter == EVFILT_WRITE) {
            printf("EVFILT_WRITE");
        } else if (filter == EVFILT_READ) {
            printf("EVFILT_READ");
        } else {
            printf("%d", filter);
        }
        printf("\n");
    }
    fflush(stdout);
    return 0;
}

The output would be:

kqfd = 3
tcp listen fd = 4
accepted fd = 5
add EVFILT_WRITE result = 0
del EVFILT_READ and EVFILT_WRITE result = -1, errno=2(No such file or directory)
kq_poll returns 1
>>> fd=5, filter=EVFILT_WRITE

This can be easily fixed, just modify the mask mask = mask & fe->mask before calling aeApiDelEvent.
I'm willing to submit a PR if this is desired.

[enjoy-binbin]

thanks, this looks reasonable, please make a PR so that we can do a review

[wkgcass]

I looked into the related code before submitting a PR, and found that the evport implementation might not be working since 2014.

This is the first commit to evport, which uses port_get(), and this commit modified it to port_getn(), both committed by Dave Pacheco in 2012. The code in aeApiDelEvent says:

We rely on the fact that our caller has already updated the mask in the eventLoop.

Then in 2014, a small commit moves aeApiDelEvent(eventLoop, fd, mask) before modifying the properties in eventLoop, which breaks the assumption which evport relies on. Also there's no real benefit or clear reason for the modification in the discussion of the the commit.

Also I believe both evport and epoll can use the mask stored in eventLoop, without re-calculating the mask themselves.

I'll submit the PR with all the above fixes I would like to make, which is still very small.
If we finally make the conclusion that only the fix to kqueue should be applied, I would still be willing to modify the PR.