Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NuProcess broken on FreeBSD 12.0 because of kevent ext field and event order #101

Open
ALPHA-60 opened this issue Jul 14, 2019 · 2 comments

Comments

@ALPHA-60
Copy link

FreeBSD 12.0 introduced the ext field in kevent struct.

Also, I noticed that events are processed in such order that process exit event is handled first, removing process from the pidProcessMap map, which in turn prevents processing of other events. This happens in a simple case of listening to output of "echo hello": both EVFILT_READ and NOTE_EXIT are retrieved at the same time (from their respective events), but exit is processed first.

I'm attaching a quick hack for FreeBSD which made most tests pass on my machine (there's still an issue of MacOS specific __pthread_chdir use). I just added the ext field and changed the order of registered events. This might be brittle, I'm not sure if the order of registered events is preserved when reporting, so the logic of processEvent might need to change instead of the events order.

diff --git a/src/main/java/com/zaxxer/nuprocess/osx/LibKevent.java b/src/main/java/com/zaxxer/nuprocess/osx/LibKevent.java
index da4990b..fd0d7b9 100644
--- a/src/main/java/com/zaxxer/nuprocess/osx/LibKevent.java
+++ b/src/main/java/com/zaxxer/nuprocess/osx/LibKevent.java
@@ -59,6 +59,9 @@ public class LibKevent
       public int fflags;
       public NativeLong data;
       public Pointer udata;
+      // "ext" field was added in FreeBSD 12.0
+      // https://svnweb.freebsd.org/base/releng/12.0/sys/sys/event.h?r1=320043&r2=316630
+      public long[] ext = new long[4];
 
       public Kevent() {
           super();
@@ -72,7 +75,7 @@ public class LibKevent
       @Override
       protected List getFieldOrder()
       {
-         return Arrays.asList("ident", "filter", "flags", "fflags", "data", "udata");
+         return Arrays.asList("ident", "filter", "flags", "fflags", "data", "udata", "ext");
       }
 
       Kevent EV_SET(long ident, int filter, int flags, int fflags, long data, Pointer udata)
diff --git a/src/main/java/com/zaxxer/nuprocess/osx/ProcessKqueue.java b/src/main/java/com/zaxxer/nuprocess/osx/ProcessKqueue.java
index b524f56..91e2c6b 100644
--- a/src/main/java/com/zaxxer/nuprocess/osx/ProcessKqueue.java
+++ b/src/main/java/com/zaxxer/nuprocess/osx/ProcessKqueue.java
@@ -101,14 +101,14 @@ final class ProcessKqueue extends BaseEventProcessor<OsxProcess>
          // We don't use the processEvents array here, since this method is not
          // called on the event processor thread.
          Kevent[] events = (Kevent[]) new Kevent().toArray(4);
-         // Listen for process exit (one-shot event)
-         events[0].EV_SET((long) pid, Kevent.EVFILT_PROC, Kevent.EV_ADD | Kevent.EV_RECEIPT | Kevent.EV_ONESHOT,
-                          Kevent.NOTE_EXIT | Kevent.NOTE_EXITSTATUS | Kevent.NOTE_REAP, 0L, pidPointer);
          // Listen for stdout and stderr data availability (events deleted automatically when file descriptors closed)
-         events[1].EV_SET(stdoutFd, Kevent.EVFILT_READ, Kevent.EV_ADD | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
-         events[2].EV_SET(stderrFd, Kevent.EVFILT_READ, Kevent.EV_ADD | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
+         events[0].EV_SET(stdoutFd, Kevent.EVFILT_READ, Kevent.EV_ADD | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
+         events[1].EV_SET(stderrFd, Kevent.EVFILT_READ, Kevent.EV_ADD | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
          // Listen for stdin data availability (initially disabled until user wants read, deleted automatically when file descriptor closed)
-         events[3].EV_SET(stdinFd, Kevent.EVFILT_WRITE, Kevent.EV_ADD | Kevent.EV_DISABLE | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
+         events[2].EV_SET(stdinFd, Kevent.EVFILT_WRITE, Kevent.EV_ADD | Kevent.EV_DISABLE | Kevent.EV_RECEIPT, 0, 0L, pidPointer);
+         // Listen for process exit (one-shot event)
+         events[3].EV_SET((long) pid, Kevent.EVFILT_PROC, Kevent.EV_ADD | Kevent.EV_RECEIPT | Kevent.EV_ONESHOT,
+                          Kevent.NOTE_EXIT | Kevent.NOTE_EXITSTATUS | Kevent.NOTE_REAP, 0L, pidPointer);
 
          registerEvents(events, 4);
       } finally {
@brettwooldridge
Copy link
Owner

brettwooldridge commented Jul 31, 2019

Thanks @ALPHA-60! Do you think you could make a pull request? I think the re-ordering of the event registration is ok, but for the Kevent structure I think we need some kind of conditional logic for FreeBSD 12.0.

I am curious how it is that FreeBSD 12.0 could introduce such a structure change without breaking many existing applications? Any idea?

@ALPHA-60
Copy link
Author

ALPHA-60 commented Aug 2, 2019

Sure, no problem! I'll be able to work on it in around 2 weeks, not before. I hope that's OK.

About FreeBSD 12.0 -- well, I'm not sure. Their handbook explicitly instructs the user who upgrades to a new major revision to recompile all packages (ports). If the user is using binary packages, I guess everything is already handled when the package for a new major revision is built on their CI. Since EV_SET did not change, this should not require changes to source. So, I guess this ABI change DID break a lot of applications, but only if one forgot to update them either by recompiling, or downloading packages built for a particular OS revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants