Closed jasonkafer closed 1 year ago
I just wanted to make the point that SYS__pthread_chdir
and SYS__chdir
do really different things – the first sets the per-thread working directory, the second sets the per-process working directory. FreeBSD does not support per-thread working directory. Very few platforms do. In fact, the only platforms I know of that do are Linux and XNU (but Linux's implementation is very different from, and incompatible with, XNU's).
Setting SYS__pthread_chdir = 12
on FreeBSD is going to really confuse people because the constant name implies "per-thread working directory" but you are setting it to a value which means "per-process working directory". It is also going to introduce thread safety issues when replacing a thread-safe API with a non-thread-safe one.
It is possible that FreeBSD developers might some day add support for pthread_chdir_np
/pthread_fchdir_np
. On the one hand, they'll likely argue that it is the wrong approach, and openat
/etc are the right approach. Especially given FreeBSD supports that approach in scenarios many other platforms don't (e.g. bindat
/connectat
for Unix domain sockets). OTOH, making it easier to port software from macOS/XNU might convince them.
But, in the meantime, I think the whole approach here is wrong – since NuProcesses' macOS process startup code is based on SYS__pthread_chdir
, it is not a good base for a FreeBSD implementation.
A better approach could be something like this:
OsxProcess
into an abstract base class BsdProcess
, with two concrete subclasses – OsxProcess
and FreeBsdProcess
OsxProcessFactory
will decide at runtime whether to create an OsxProcess
or a FreeBsdProcess
depending on what the OS is (possibly should rename it to BsdProcessFactory
, but that might break backward compatibility – one option might be to do the rename, but keep OsxProcessFactory
as a dummy subclass)spawnWithCwd
method of BsdProcess
be protected abstract
, so the concrete subclasses have to implement their own spawnWithCwd
FreeBsdProcess
implementation of spawnWithCwd
, use a completely different strategy. For example, you could spawn an external helper binary written in C, which does a chdir() to its first argument, then execs the remaining arguments.LinuxProcess
does this (using the jspawnhelper
binary shipped with the JDK) – I'm not sure if that code works on FreeBSD, but if it does you could base FreeBsdProcess
more off LinuxProcess
than OsxProcess
./bin/sh
as in this: https://unix.stackexchange.com/a/480779Thanks for the detailed feedback, @skissane! I'll readily admit, I lack the depth of knowledge you have around FreeBSD and how some of these internals work there. Looking purely at end behavior, though, trying to use the per-process work directory to control working directories for processes we spawn, and can actively be spawning on multiple threads concurrently, seems like an obvious no-no. (I no longer work on Bitbucket Server, but thinking about this in terms of Bitbucket Server's usage of the library it'd be pretty disastrous to, for example, have 2 threads that are trying to operate on different repositories end up racily changing the process-wide working directory and one thread's update stomps the other, resulting in the forked git
processes both running on the same repository. The support case to try and find that issue would be brutal.)
Based on that potential issue, and reinforced given that I asked for a variety of rework on this pull request over 3 years ago and it hasn't been actioned, I think the right approach is to decline this change and consider other options separately. One thing I'll add to that, though, is that I don't a) use FreeBSD or b) have any sort of FreeBSD setup, so I personally am unlikely to make any inroads on a solution to this issue.
Fix for FreeBSD LibC chdir syscal