Don't use the systemd plugin on JRuby #3079
Conversation
The systemd integration will fail for JRuby at https://github.com/puma/puma/blob/e3d5794a7ebe47577ced4d4dfdd6a6cc969ded01/lib/puma/sd_notify.rb#L140, because JRuby doesn't support UNIX datagram sockets yet, and won't for a while. See jruby/jruby#6504. So turning it off here, so that JRuby users can integrate with systemd on their own if they wish without errors.
lib/puma/launcher.rb
Outdated
| @@ -59,7 +59,7 @@ def initialize(conf, launcher_args={}) | |||
|
|
|||
| @environment = conf.environment | |||
|
|
|||
| if ENV["NOTIFY_SOCKET"] | |||
| if ENV["NOTIFY_SOCKET"] && RUBY_PLATFORM != "java" | |||
There was a problem hiding this comment.
Makes sense seeing the tests are skipped on JRuby:
puma/test/test_plugin_systemd.rb
Line 13 in e3d5794
I do wonder though, can we make the skip more specific?
Similar to Puma::HAS_UNIX_SOCKET / skip_unless :unix:
Line 29 in e3d5794
(and also skip the tests based on this?)
There was a problem hiding this comment.
@mohamedhafez Can we at least add a comment why we exclude JRuby here?
Should probably also make use of the detection Puma already has?
| if ENV["NOTIFY_SOCKET"] && RUBY_PLATFORM != "java" | |
| if ENV["NOTIFY_SOCKET"] && Puma.jruby? |
There was a problem hiding this comment.
Didn't know about Puma.jruby?, added that in (but it needs to be !Puma.jruby?). Also good call with the comment, so people know why this was needed in the future, and also I put the issue in the comment so that if anyone in the future is considering removing it they can double check if the issue was resolved easily.
|
Just to be clear, there's a use-case for running under JRuby, setting |
|
Sounds like a missing test case... :) |
|
Without this patch, it will break JRuby running under systemd when NOTIFY_SOCKET is set (i.e. when the service All the constants are defined in JRuby, they just don't work correctly when talking to unix datagram sockets, so anything like |
|
As far as a use case of why I would want run a i.e. I'm taking care of it on my own, and just don't want puma to crash trying to handle it in a way that doesn't work on JRuby. |
|
Could someone with permission label this a bug fix so that the tests will pass? |
Added a comment to explain the situation, and used Puma's JRuby detection method instead of re-coding it.
|
What do you think about adding a test that runs on JRuby, that sets I'm thinking if the JRuby issue is resolved, its very easy to just remove the conditional and think everything is good, but if something fails when you do that, you may remember to remove the "skip jruby" for the systemd plugin tests. |
|
@dentarg sounds like a good idea, but I'm not sure how to accomplish that without just making sure things don't crash when you set Perhaps instead in the comment I can just put "If the issue is ever fixed and you remove the && !Puma.jruby?, please remember to fix the systemd tests to run on JRuby as well". It'll be a lot of comments for a small change, but it won't hurt anything 🤷♀️ Open to any suggestions. Is there a way to check that a plugin isn't loaded with a test? Like can we stub it out and have it set a variable, and then check if that variable was set or something? Sorry I'm not super familiar with Ruby testing and just thinking off the top of my head |
Yeah, a bit hacky but: |
|
Ok, so I'm thinking make a require_relative "helper"
require_relative "helpers/integration"
class TestJRubySkipPluginSystemd < TestIntegration
THREAD_LOG = TRUFFLE ? "{ 0/16 threads, 16 available, 0 backlog }" :
"{ 0/5 threads, 5 available, 0 backlog }"
def setup
skip "Skipped because Systemd support is linux-only" if windows? || osx?
skip_unless :unix
skip_unless_signal_exist? :TERM
skip_unless :jruby
super
ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
end
def test_systemd_skipped
cli_server "test/rackup/hello.ru"
assert_nil Puma::Plugins.instance_variable_get(:@plugins)["systemd"]
stop_server
end
end Does that sound about right? Or would you prefer I get it into test_plugin_systemd and then add |
|
The separate file seems resonable to me 👍 (didn't realise we skip jruby in the setup, in |
|
Looks like it worked! You can see the new test being run in the JRuby tests, but not the MRI or TruffleRuby ones. Just seems like there was some kind of failure on an unrelated test, should i just make a dummy push to try it again, or does this look good to you considering the failure has nothing to do with this, and was passing earlier? |
|
Labelling bug because I do think this is a bug, and this way it will show up in the next version's release notes. |
|
renamed the test because having "skipped" in the name of the test made me think the actual test itself was skipped for a second, and i figure that could be confusing to others. This time we should get all the tests passing we'll see in a sec |
* Don't use the systemd plugin on JRuby The systemd integration will fail for JRuby at https://github.com/puma/puma/blob/e3d5794a7ebe47577ced4d4dfdd6a6cc969ded01/lib/puma/sd_notify.rb#L140, because JRuby doesn't support UNIX datagram sockets yet, and won't for a while. See jruby/jruby#6504. So turning it off here, so that JRuby users can integrate with systemd on their own if they wish without errors. * Improved skipping systemd for JRuby Added a comment to explain the situation, and used Puma's JRuby detection method instead of re-coding it. * test that systemd plugin isn't loaded on JRuby * rename to test_plugin_systemd_jruby.rb, fix lint * rename test to test_systemd_plugin_not_loaded * make and use skip_unless :linux
The systemd integration will fail for JRuby at
puma/lib/puma/sd_notify.rb
Line 140 in e3d5794