Change ident for EVFILT_USER to 0 and add a test #1582
Conversation
Conventionally, ident for EVFILT_USER is set to 0 to avoid collision of file descriptors, which is what other renowned networking frameworks like netty(java), mio(rust), gnet(go), swift-nio(swift), etc. do currently.
|
Hi @azat, got a minute for this? |
|
Kindly ping @azat |
Yes, this change has a practical effect. You see, we added
True, but 0 represents stdin and it's ideal for the
|
|
Besides, I think this |
Interesting, can you provide a reproducer with raw kqueue maybe?
But don't we hit the same issue if will add an event for 0 fd? Maybe what do you think about using INT_MAX for this purpose? |
Actually, I can't assure you that's the root cause, just take a shot :)
I don't quite get you here, why would we add a 0 fd?
I don't think this is a good idea given that |
To monitor events on STDIN? I.e.
Correct, but this fd will not be added to to evmap, only to kqueue for monitoring |
You're suggesting that we create a special branch in |
|
The special treatment is not needed, NOTIFY_IDENT is added only to kqueue, it does not leave the kqueue.c, so it is not added to evmap |
This sounds really interesting, I would love to see at the example that will show this problem! |
Oh，right. I misread your comment. I think it's feasible. |
|
And I would really want to understand is it a problem or not (I'm not sure about this actually), so before, can you please try to reproduce it, and then a) we can add a test and b) write a sensible comment for this constant and commit |
This can be a tricky case to reproduce, but I'll try. |
|
It turns out that unlike Demo code#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <pthread.h>
#include <sys/types.h>
#include <sys/event.h>
#include <sys/time.h>
#include <string.h>
#define NOTIFY_IDENT 42
void *writer_thread(void *arg) {
int *fd = (int *)arg; // Get file descriptor from argument
sleep(1); // Let things settle before writing
const char *message = "Hi!";
write(*fd, message, strlen(message));
printf("Wrote data to pipe...\n");
return NULL;
}
void *trigger_thread(void *arg) {
int *fd = (int *)arg; // Get file descriptor from argument
struct kevent ev;
struct timespec timeout = { 0, 0 };
EV_SET(&ev, NOTIFY_IDENT, EVFILT_USER, 0, NOTE_TRIGGER, 0, NULL);
sleep(1); // Let things settle before writing
if (kevent(*fd, &ev, 1, NULL, 0, &timeout) == -1) {
perror("Error triggering event");
//exit(1);
return NULL;
}
printf("Triggered user-level event...\n");
return NULL;
}
int main() {
int kq = kqueue();
if (kq == -1) {
perror("kqueue");
exit(1);
}
printf("kqueue=%d\n", kq);
int pipefd[2];
if (pipe(pipefd) == -1) {
perror("pipe");
exit(1);
}
printf("pipefd[0]=%d, pipefd[1]=%d\n", pipefd[0], pipefd[1]);
// Overwrite pipe read-end with fd 42
if (dup2(pipefd[0], NOTIFY_IDENT) == -1) {
perror("dup2");
exit(1);
}
close(pipefd[0]);
pipefd[0] = NOTIFY_IDENT;
// Add the pipe read-end for monitoring read events
struct kevent ev2;
EV_SET(&ev2, NOTIFY_IDENT, EVFILT_READ, EV_ADD, 0, 0, NULL);
kevent(kq, &ev2, 1, NULL, 0, NULL);
// Initial registration for user-level events
struct kevent ev1;
EV_SET(&ev1, NOTIFY_IDENT, EVFILT_USER, EV_ADD | EV_CLEAR, 0, 0, NULL);
kevent(kq, &ev1, 1, NULL, 0, NULL);
// Writer thread
pthread_t writer;
pthread_create(&writer, NULL, writer_thread, &pipefd[1]); // Pass the fd of pipe write-end
// Trigger thread
pthread_t trigger;
pthread_create(&trigger, NULL, trigger_thread, &kq);
pthread_join(trigger, NULL);
pthread_join(writer, NULL);
// Blocking kevent for 10s if no events happen
struct timespec timeout = { 10, 0 };
struct kevent events[2];
int n = kevent(kq, NULL, 0, events, 2, &timeout);
printf("kevent returned %d events\n", n);
for (int i = 0; i < n; i++) {
printf(" ident: %ld, filter: %d, flags: %d, fflags: %d\n",
events[i].ident, events[i].filter, events[i].flags, events[i].fflags);
}
close(pipefd[0]);
close(pipefd[1]);
close(kq);
return 0;
}Result: |
panjf2000
changed the title
Co-authored-by: Azat Khuzhin <[email protected]>
|
I kept getting this error on macOS m1 while building the new test: I don't know what I'm missing, maybe you do? @azat |
|
Any clues? @azat |
|
Got a minute for this? @azat |
You need to link event_pthreads for your example. |
|
Build is broken: |
|
Kindly ping @azat |
* upstream/master: Bump the github-actions group with 5 updates
Thanks, but I don't think CI linux-autotools-job is happy about this. Should we be worried about that? |
|
Definitely yes |
|
I've fixed the autotools failures. @azat |
* upstream/master: Do not set TCP keepalive on Unix sockets Fix some comments Avoid calling read(2) on eventfd on each event-loop wakeup
|
Not completely:
Fixed. And I hope now test.sh is more robust |
|
cmake was also broken, it simply does not run new test - fixed |
|
The CIs of libevent are extremely slow, I noticed that the regress tests took the most time, which makes the PR progress very inefficient, are there any improvements we can do to speed the tests up? @azat |
Yes, sadly, but this is a very know issue, especially for macos/windows, since github provides less machine time for public use
Yes, there is at least one thing that can be done #1452 Another thing is to setup own private workers, so that we will have more resources, but this is whole another story. |
|
I think we're good to go? @azat |
Conventionally,
identforEVFILT_USERis set to 0, which is what other renowned networking frameworks like netty(java), mio(rust), gnet(go), swift-nio(swift), etc. do currently.