victori has quit [Read error: Connection reset by peer]
victori has joined #jruby
sagax has quit [Quit: Konversation terminated!]
ur5us_ has joined #jruby
ur5us has quit [Ping timeout: 250 seconds]
ur5us_ has quit [Ping timeout: 240 seconds]
ur5us_ has joined #jruby
ur5us_ has quit [Ping timeout: 258 seconds]
drbobbeaty has quit [Ping timeout: 246 seconds]
drbobbeaty has joined #jruby
<headius[m]>
ok so the ProcessBuilder approach seems like the best option... need to decide if we can drop the native popen3 and also just use ProcessBuilder for that
<headius[m]>
I am leaning toward ditching all of our hacked-together logic to make Process and the crappy IO streams it gives us work right... we should just fall back to using them as is and if you want proper functionality you need native support
<headius[m]>
i.e. I want to delete ShellLauncher this year
<headius[m]>
or strip it down to just a minimal ProcessBuilder wrapper
victori has quit [Ping timeout: 260 seconds]
<headius[m]>
enebo: I will push a PR for this shortly... trying to keep this change minimal but these methods did not work at all without native process support
victori has joined #jruby
<headius[m]>
darn, all the open3 tests themselves require native file descriptors
<headius[m]>
one of those is an interface, the other is the Java 8 version of CHM which does not require setting a concurrency level
<headius[m]>
I assume it was included because prior to Java 8, not specifying a low concurrency level would default to # of cores and end up very large in memory
<headius[m]>
Unsafe is being deprecated and removed bit by bit in newer JDKs
<chrisseaton[m]>
Ah and this code lives outside the safe bubble as it's been removed from the core.
<headius[m]>
I don't know how concurrent-ruby decides to use one or the other
<headius[m]>
well, this is public domain code that was implemented outside JDK, but it used Unsafe to avoid the overhead of AtomicReferenceFieldUpdater
<chrisseaton[m]>
I was amazed the other day - I was looking into optimising field reads and writes done via unsafe, and the JIT actually turns unsafe reads and writes on objects back into normal field accesses if it can!
<headius[m]>
all Unsafe methods are intrinsics on Hotspot and I assume other VMs
<chrisseaton[m]>
So for example using unsafe on an object does not stop it being scalar replaced
<headius[m]>
ah, sure... I would hope not
<headius[m]>
ironically it probably helps escape analysis because it will skip access checks and other branches that confound the simple EA in hotspot
<headius[m]>
enebo: so I am running open3 tests with this non-native impl and making pretty good progress
<headius[m]>
we may want to start introducing some targeted non-native test runs
<enebo[m]>
headius: yeah
<headius[m]>
I think I am down to one failure and two hangs but I am rerunning the hangs because other fixes might have resolved them
<headius[m]>
ah yes... so the two that hang require passing a file descriptor through to the process launch, which we obviously can't do with Java ProcessBuilder
<headius[m]>
so they might pass if not for that
<headius[m]>
so
<headius[m]>
Finished tests in 88.472445s, 0.3052 tests/s, 1.8537 assertions/s.
<headius[m]>
people asked us to get these methods working years ago... many years
<headius[m]>
now I feel bad that I didn't attempt it
<headius[m]>
I just assumed it would never work well enough with Process's buffered pipes and such
<enebo[m]>
yeah process has been a nightmare in general too
<headius[m]>
so my big fix here was realizing that Channels.newChannel(outputStream) does not flush the stream
<headius[m]>
which seems like a freaking giant bug since Channels are supposed to be unbuffered
<headius[m]>
enebo: so I got the last one to pass but it was due to being unable to unwrap Process to get the pid... we would need to add-opens for java.lang for that and that would need some discussion
<enebo[m]>
I am half suprised that is not on the list already
<headius[m]>
I was too but I vaguely remember being afraid to open that one up
<headius[m]>
I can open an issue for that and we can debate it but I think we need to have a better grip on module visibility before then
<headius[m]>
e.g. currently calls from Ruby look like calls from jruby.dist so they gain our add-opens
<headius[m]>
it would not be difficult to assign them their own runtime module but some finesse would be needed to keep reflective calls we want to work, working
<headius[m]>
I have a gaggle of commits landing momentarily
<headius[m]>
additional fixes have been pushed there
<headius[m]>
I don't think there is any rush on this until someone reports that they are having issues with native gem installs on Windows
<enebo[m]>
yeah I guess we can figure out how we can harden this so we can include it onto 9.2
<enebo[m]>
Not sure how to deal with these sorts of issues because sassc obviously will take ot 9.2 from the windows build matrix
<enebo[m]>
but I would hate to break other stuff
<headius[m]>
so the open3 changes are pretty low risk... they would have just been hard failures before
<headius[m]>
I do flip popen3 to using the new logic but it passes more open3 tests as well
<headius[m]>
so the other possibly concerning change would be the new "SyncOutputStreamChannel" where we used the JDK's broken one before
<headius[m]>
we should talk through this PR though when you get a chance
<headius[m]>
enebo: open3 PR is basically ready... I am bringing up my Windows VM to test that sassc installs ok
<enebo[m]>
coolio
<headius[m]>
I suppose I should have make installed
<headius[m]>
success!
<headius[m]>
enebo: I am making one additional change, making this overlay the open3 in stdlib with just the pieces it replaces
<headius[m]>
should be zero-sum
<enebo[m]>
so the comment changes and stuff to windows_open3 is that you applying some other ruby code?
<headius[m]>
it was me updating the copied impls from open3 to open3_windows but now I am just removing anything that is duplicated
<headius[m]>
so it will load the default open3.rb and then if on Windows it will load open3_windows to patch over the popen methods
<headius[m]>
that is sufficient to get it working
<headius[m]>
enebo: I have pushed the change, should reduce the overall diff
<enebo[m]>
ok
<headius[m]>
oh wait, did not push, one sec
<headius[m]>
had to double check passing
<headius[m]>
enebo: ok really there now
<headius[m]>
come at me
<headius[m]>
ahh one thing about this PR... the ShellLauncher changes are good but do not fix anything in here now
<headius[m]>
because the open3 popen methods have all been replaced that fix is no longer needed... could be removed but it adds leading env hash support to some non-native process spawns
<headius[m]>
it just wasn't sufficient to fix the problem
<enebo[m]>
someone pointed out a flush is not needed if 0 bytes are written which may not really be worth it or we are guaranteed to get >0?
<headius[m]>
I am on the fence having just dealt with Ruby IO wrappers that lie about how many bytes were written
<headius[m]>
only things I can think of that would be a problem would be any locking or overhead from flushing that could be avoided
<headius[m]>
but one of my arguments to core-libs-dev was also that this flush adds immediacy to any errors the underlying IO would raise, so that is argument for always flushing
<enebo[m]>
This is because builder will have an existing env from our internal process but the env will essentially merge with it
<headius[m]>
this is because of ProcessBuilder API... the way to set env is to get env and then modify it
<headius[m]>
it is not a good API
<enebo[m]>
ok
<enebo[m]>
I don't actually find the mandatory flush to be a significant issue other than perhaps MRI will not flush as much
<headius[m]>
well in MRI these would be direct sync IOs so I have to emulate that much
<headius[m]>
they do not flush but they do not buffer
<enebo[m]>
haha ok well year if it isn't buffered then flushing is pretty needed
<enebo[m]>
ok yeah I am not seeing much more here and it looks like an improvement but it is a fair amount of code
<enebo[m]>
The fact it fixes the problem on windows and some more stuff works is great
<enebo[m]>
but other OSes will experience some of this code right? The ShellLauncher changes
<enebo[m]>
and WritableChannel
<headius[m]>
heh I guess that did not reduce diff because I deleted a bunch
<headius[m]>
look at open3_windows alone and you will see it better
<enebo[m]>
well it is still a helpful diff this way
<enebo[m]>
it is pretty clear even with the massive emptiness what is happening now
<headius[m]>
the open3_windows file was originally used on all platforms but then once Windows was the only one without native processes it got copied over
<headius[m]>
so this tidies that up and makes it a simple overlay
<enebo[m]>
Other than evaluating non-Windows Channel change I don't see any issue
<enebo[m]>
I can hit windows a bit too before release to find issues there
rtyler has joined #jruby
<rtyler>
Greetings again friends, I'm having trouble formulating a search query here, but I wonder if there's an environment variable I can pass to a Java process which is running JRuby to execute a referenceed .rb file
<rtyler>
almost like a LD_PRELOAD type of deal
<headius[m]>
rtyler: hey ltns
<headius[m]>
I think we might have something like that
<headius[m]>
execute as in $0?
<headius[m]>
because requires should honor the usual load path env vars
<headius[m]>
jeremyevans: you said reverse backtraces were reverted here but the referenced hash does not seem to do that... do you have a better link? https://bugs.ruby-lang.org/issues/14558
<byteit101[m]>
when testing the interface inheritance, it "works" until I new it after reification, then I get exciting errors: Java::JavaLang::ClassCastException (org.jruby.RubyModule$INVOKER$i$0$0$initialize cannot be cast to org.jruby.internal.runtime.AbstractIRMethod)
<byteit101[m]>
I am impressed it got that far though, I think "new" is not the right new
<byteit101[m]>
Will investigate later though, gotta run in 3 min