Added Processes to IONative #3575
Conversation
|
@armanbilge can you please review this PR |
There was a problem hiding this comment.
Thanks, nice work! A few quick thoughts:
- Let's try to get CI green. The native bindings you introduced may need
@nowarnannotations (there should be other examples in the codebase). - I think we can unguard some
ProcessSuitetests, at least on Linux. - We may want to keep the
java.lang.Processimplementation in shared sources, and use it as a fallback on Native for non Linux/macOS (i.e. Windows).
|
I checked the timeout exception in the env inheritance test. I think the issue comes in how the envMap/envp is being constructed. Interestingly, adding a dummy variable when the Map is empty seems to solve the problem. |
|
@antoniojimeneznieto Yep, I tried using dummy variable, and it actually resolved the issue. Also, I’m not fully understanding why the EOF signal isn’t being triggered in |
|
Yes! this isn't a fix, since it may introduce other issues or conflicts. It was more of a remark than a proper solution, sorry for the confusion. |
|
@armanbilge @antoniojimeneznieto I have found the issue, The epollSystem was not including |
rahulrangers
force-pushed
the
native-process
branch
from
01ee5ad to
00ac3e5
Compare
|
@armanbilge can you please review this PR |
rahulrangers
force-pushed
the
native-process
branch
from
1ae5378 to
d410dee
Compare
There was a problem hiding this comment.
I am sorry it took me so long to give this the review it deserves! Overall this looks very good and so happy to seem so many tests enabled in the test suite :) great job with that
I do have a quite a bit of feedback but it is mostly polish. Also, a general advice is to use more of the helpers like guard_ and errnoToThrowable in NativeUtil.
io/jvm-native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
io/jvm-native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
|
|
||
| @extern | ||
| @nowarn212("cat=unused") | ||
| object SyscallBindings { |
There was a problem hiding this comment.
| object SyscallBindings { | |
| private object syssyscall { |
There was a problem hiding this comment.
We can move the bindings to fs2/io/native/src/main/scala/fs2/io/internal
io/native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
io/native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
| @inline private def closeAll(fds: Int*): Unit = | ||
| fds.foreach { fd => close(fd); () } | ||
|
|
||
| def forAsync[F[_]: LiftIO](implicit F: Async[F]): Processes[F] = |
There was a problem hiding this comment.
| def forAsync[F[_]: LiftIO](implicit F: Async[F]): Processes[F] = | |
| def forLiftIO[F[_]: LiftIO](implicit F: Async[F]): Processes[F] = |
io/native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
| } else { | ||
| val status = stackalloc[CInt]() | ||
| waitpid(proc.pid, status, WNOHANG) | ||
| () |
There was a problem hiding this comment.
What is the purpose of this else? Since it doesn't seem to use the status.
There was a problem hiding this comment.
we are having else to remove the zombie process from the kernel space as given in this:
A child that terminates, but has not been waited for becomes a
"zombie".
https://man7.org/linux/man-pages/man2/waitpid.2.html
io/native/src/main/scala/fs2/io/process/ProcessesPlatform.scala
Outdated
Show resolved
Hide resolved
rahulrangers
force-pushed
the
native-process
branch
from
d410dee to
600671c
Compare
rahulrangers
force-pushed
the
native-process
branch
from
9f11811 to
b007fbd
Compare
Implemented ProcessBuilder-based process spawning for Scala Native using posix_spawn, with support for stdin, stdout, and stderr piping. Integrated pidfd_open and fileDescriptorPoller to asynchronously and non-blockingly wait for process termination. Falls back to waitpid when pidfd_open is unavailable.