remove usage of jruby-stdin-channel

[colinsurprenant]

The interruptible stdin implementation in in https://github.com/colinsurprenant/jruby-stdin-channel is not required anymore in this plugin, the current JRuby stdin in now interruptible.

Relates to elastic/logstash#12071

[yaauie]

There is still an open issue on jruby about stdin's interruptibility, noting that support may have gotten better on linux/darwin, but that there may still be issues with Windows: jruby/jruby#2024

[yaauie]

Assuming that the upstream issues are reliably fixed for all platforms, the changes here look sensible.

I've added a note about explicitly depending on either the Logstash core or the Jruby that provided the fix to ensure those who still need the fix do not resolve to a version of this plugin that still needs it.

[yaauie]

Do we know precisely which version of Logstash and/or Jruby is needed in order to preclude the need for jruby-stdin-channel? If so we should add that as a requirement so that this update isn't pulled in on Logstashes/Jrubies that still need it.

[kares]

👍 believe the changes are due to the JRuby 9K IO sub-system rewrite.
so any 9.x should do, either logstash-core >= 6.0 or simply s.required_ruby_version '>= 2.3'

[kares]

There is still an open issue on jruby about stdin's interruptibility, noting that support may have gotten better on linux/darwin, but that there may still be issues with Windows: jruby/jruby#2024

missed this one, I think it's still an issue - even on Linux/Mac.
we might need a spec checking for in.read followed by an in.close not causing a hang (till a char is received).

[colinsurprenant]

@yaauie @kares good observations 👀

[colinsurprenant]

@kares yeah, I will add a spec for the interruptibility, good suggestion.

One thing though, I don't think that plugin specs are run on different platforms are they? Plus we are not running all default plugins specs included in LS as part of the LS build (which does multiple platforms).

Any suggestions for that? /cc @jsvd If at least we could have a spec which would indicate what the expected behaviour is with some possible exceptions per platform (Windows) and if that changes it could indicate to look at the stdin plugin to make some changes or something like that?

[kares]

if needed we could setup Travis CI's matrix to also run tests on a Windows box.
however, I believe jruby/jruby#2024 is still a blocker for LS regardless of the OS.

[colinsurprenant]

@kares that's weird. I did test interruptibility in LS using this PR patched version of the input plugin and it worked. Let me see what I missed here or what the difference is 🤔

[colinsurprenant]

Got it! @kares as discussed in jruby/jruby#2024 - your test uses $stdin.read but LS used $stdin.sysread which now seems to be interruptible. Figuring out when/where that change occurred in JRuby.

[colinsurprenant]

Ok, so the change in behaviour seems to have happened in 9.1.15.0 so I believe starting at v6.7.0 when we upgraded JRuby from 9.1.13.0 to 9.2.6.0 we did not need the jruby-stdin-channel fix anymore.

As suggested I could a dependecy such as

s.add_runtime_dependency "logstash-core", ">= 6.7.0"

I'll also add a spec to test interruptibility to avoid future regression on that.

@kares @yaauie LMKWYT.

[colinsurprenant]

Added logstash-core version constrain and a spec. I tested the spec on JRuby version < 9.1.15.0 and it fails on the timeout.

[kares]

thanks for explaining the details - have assumed if read hangs sysread would behave the same, apparently isn't the case 😜
believe the sysread started to change at jruby/jruby@d8ef3bd#diff-f27dd7bfdf08a6e0ac098956f937f830R2514 (there's more commits following up) which boils down to a posix.read but before that there's a select to wait for the channel.

so this is starting to look good, esp. since there's a specs guarding against potential regressions (hangs).

however, I do not think the IO logic is the same on Windows - not sure about the exact details but my guess is that the channel won't come back as selectable. any how I did a quick test which confirms sysread does behave (like read) non-interruptible on Windows (tested JRuby 9.2.11.1)

[colinsurprenant]

@kares thanks!
Thanks for the Windows heads up. We should probably flag this in JRuby?
In the meanwhile we might be stuck at keeping jruby-stdin-channel around for Windows. What triggered this PR was the push to remove the An illegal reflective access warning(s). The easiest is to simply remove that code and it was perfect that we realized it was not needed anymore but that was not accounting for Windows.
Now, I wonder if we can refactor that code to avoid the illegal reflective access ... I'll dig a bit into that, let me now if anyone has ideas around that.

[kares]

Thanks for the Windows heads up. We should probably flag this in JRuby?

Yeah, was thinking about that but than there's jruby/jruby#2024 already.
On the other hand this is a platform compatibility issue regarding sysread not being interruptible on Windows.