Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using at_exit for graceful stop upon receiving a SIGINT in JRuby #1868

Closed

Conversation

Adithya-copart
Copy link

@Adithya-copart Adithya-copart commented Jul 28, 2019

This resolves #1675.
Sorry about the noise in that issue but you can find a sample app here demonstrating the problem.

The graceful_stop is not using the same thread as the at_exit blocks and is being run in the JVM signal handling thread in JRuby.

So, the thread.join here is joining @thread on the signal handler thread and I'm not sure if that is the intention.

This fix will execute graceful_stop within the at_exit block on the main thread.

Link: jruby/jruby#5437

@nateberkopec
Copy link
Member

Needs a test. Probably just an adaptation/simplification of your reproduction app.

@Adithya-copart
Copy link
Author

@nateberkopec I'm not sure how to come up with a working test.
I'm running into an issue after adding a test to test_integration.rb.

The problem is executing the following code in a irb session will block at Process.wait(server.pid) when running on master but not in this branch.

However, if I add the same code to a file and run it, the puma process is shutting down properly on both the branches. I'm not sure what to make out from this.

require 'open3'

server = IO.popen("rackup test/rackup/jruby-sigint.ru")

# Wait for bootup
true while server.gets !~ /Ctrl-C/

t1 = Time.now
Process.kill(:INT, server.pid)
Process.wait(server.pid)
p "Time elapsed: #{Time.now - t1}"

@Adithya-copart
Copy link
Author

pid = spawn("rackup test/rackup/jruby-sigint.ru")

# Wait until bootup
sleep 6
t1 = Time.now
Process.kill(:INT, pid)
Process.wait(pid)
p "Time elapsed: #{Time.now - t1}"

This works inside a file, I'll see if I can integrate this into a test instead.
I might need to make sure the teardown works if the server fails to shutdown.

@Adithya-copart
Copy link
Author

Faced jruby/jruby#1050 (comment) with an approach using Timeout.timeout and Process.wait.

So, I came-up with a test that fails on master and passes in this branch but it does use sleep.

@Adithya-copart
Copy link
Author

@nateberkopec I opened jruby/jruby#5844 to understand #1868 (comment).

@headius investigated the issue and added jruby/jruby#5844 (comment).

This piece of code may not longer be needed in Puma. It was initially added in cli.rb via 1dbe28e a long time ago and was later moved to launcher.rb.

#1675 no longer happens when I remove this code. The puma server shutdown as expected.

puma/lib/puma/launcher.rb

Lines 408 to 413 in 9fb1228

Signal.trap "SIGINT" do
if Puma.jruby?
@status = :exit
graceful_stop
exit
end

@nateberkopec
Copy link
Member

@Adithya-copart It looks like Evan added that code in response to this comment: #73 (comment)

What do you think? I guess Ctrl-C still shuts down the JVM right? Is that what you're seeing?

@Adithya-copart
Copy link
Author

Yep, Ctrl+C is shutting down the JVM without that piece of code.

Tested on my Mac and a JRuby docker container.

@nateberkopec
Copy link
Member

Does it remove the socket still, though? I guess it must if the tests are passing. Originally that's why this code was added, ctrl-c just blew up the JVM without properly removing the socket.

@nateberkopec
Copy link
Member

@Adithya-copart maybe the test should check to make sure the socket is removed?

Sent with GitHawk

@nateberkopec nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Aug 29, 2019
Adithya Pentela added 7 commits September 1, 2019 14:27
WIP: The server shuts down properly inside the test case when the graceful_stop is executed on the signal handler thread.

Fix rubocop offenses and add a test

Fix rubocop offenses and add a test

Prevent teardown in test_integration.rb from trying to kill an already killed server
@Adithya-copart
Copy link
Author

Updated the test to check if the port is available after shutdown.
Let me know if this is not what you had in mind.

@nateberkopec nateberkopec removed the waiting-for-changes Waiting on changes from the requestor label Sep 11, 2019
nateberkopec added a commit that referenced this pull request Sep 11, 2019
See #1868

The original intent of the trap was to ensure that the socket was removed
properly. It now does, so the trap is no longer necessary.
@nateberkopec
Copy link
Member

Closing in favor of #1961

nateberkopec added a commit that referenced this pull request Sep 11, 2019
See #1868

The original intent of the trap was to ensure that the socket was removed
properly. It now does, so the trap is no longer necessary.
dentarg added a commit to dentarg/puma that referenced this pull request Sep 23, 2019
Use the test case from puma#1868

Looks like puma#1961 does not test with a
background thread
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Puma does not shutdown gracefully
2 participants