Skip to content

Compatibility fixes for open3 on Windows#6649

Merged
headius merged 8 commits intojruby:jruby-9.2from
headius:fix_nonnative_spawn_with_env
Apr 6, 2021
Merged

Compatibility fixes for open3 on Windows#6649
headius merged 8 commits intojruby:jruby-9.2from
headius:fix_nonnative_spawn_with_env

Conversation

@headius
Copy link
Member

@headius headius commented Apr 2, 2021

This is a series of fixes to improve the behavior of the open3 library when run on platforms where we cannot support native process launching (currently just Windows).

The bulk of changes here rewrite the non-native open3 shim to use java.lang.ProcessBuilder. Environment hash and options hash are also better supported.

@headius headius added this to the JRuby 9.2.18.0 milestone Apr 2, 2021
@headius headius force-pushed the fix_nonnative_spawn_with_env branch from 751bbde to 390d93f Compare April 2, 2021 20:46
headius added 6 commits April 2, 2021 16:06
The API here is similar enough that this is an easier match than
trying to refit our ShellLauncher for these other forms. This
change allows the sassc gem extension build to succeed without any
native support loaded (or without native process support as on
Windows).
This was changed sometime between 8 and 11 from UNIXProcess to
ProcessImpl. Note that while this exposes the proper class, it
still will not work without opening java.base/java.lang to JRuby.
Channel is intended to be an unbuffered IO construct backed
directly by a native file descriptor, but it has subclasses that
wrap other types of IO streams. In the case of OutputStream, the
wrapper seems to have a crucial flaw: it does not flush the
underlying stream. Since Channel provides no way to flush, the
buffered data will sit there forever.

This change modifies our logic to use a "sync" version of the
OutputStream channel wrapper, forcing the underlying stream to
flush after each write operation.
These fixes bring test_open3 down to only two hanging specs, which
hang due to the inablity to pass raw file descriptors through
ProcessBuilder into the eventual subprocess.

* Properly propagate options through all methods.
* popen2 should inherit stderr from parent.
* The wait thread should mimic a Process.detach thread with pid
  method and thread-local value.
* The process exit status should be shifted right 8 bits to mimic
  native packed process results.
* Move Open3.popen3 to using this new logic rather than the
  one-off custom IO.popen3 we implemented years ago. The new
  logic behaves better.
* Avoid kwrest arguments since they behave poorly in Ruby 2.6 and
  earlier.
* Set process streams to sync. This works together with 5a6912c
  to ensure that the wrapped OutputStream does not buffer data.
@headius headius changed the title Support leading env hash in nonnative process spawn Compatibility fixes for open3 on Windows Apr 6, 2021
@headius
Copy link
Member Author

headius commented Apr 6, 2021

Note that the new SyncOutputStreamChannel is working around what I believe is a flaw in the default wrapper, detailed in an email to the OpenJDK core-libs-dev list here:

https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-April/076025.html

@headius
Copy link
Member Author

headius commented Apr 6, 2021

This now fixes #6648, tested on Windows 10 with msys/mingw64 tooling installed.

@headius headius linked an issue Apr 6, 2021 that may be closed by this pull request
@ahorek
Copy link
Contributor

ahorek commented Apr 6, 2021

I tested it also on 9.3 and you need to add require 'jruby' because JRuby.runtime doesn't work without it, but other than that it looks great.

thanks @headius

@headius
Copy link
Member Author

headius commented Apr 6, 2021

@ahorek Great catch, thanks!

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

Successfully merging this pull request may close these issues.

JRuby 9.2.17.0 fails to install sassc on Windows

2 participants