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

Allow shutdown_pipe to be passed in via @config #44

Merged
merged 2 commits into from
Jul 21, 2020

Conversation

wishdev
Copy link
Contributor

@wishdev wishdev commented May 30, 2020

I would like to be able to fork a webrick server and then shut it down from the parent process.

The @shutdown_pipe variable within webrick/server is the perfect tool for the job but it is not settable/accessible outside of the class. This patch simply allows for a pipe to be passed in via the server config.

@jeremyevans
Copy link
Contributor

I think it's fine to expose this, but we should add a test for it to prevent regressions.

@wishdev
Copy link
Contributor Author

wishdev commented Jul 21, 2020

Test added - thanks for your time Jeremy!

@jeremyevans
Copy link
Contributor

Thanks for working on this!

@jeremyevans jeremyevans merged commit 1daacc1 into ruby:master Jul 21, 2020
@jeremyevans
Copy link
Contributor

@wishdev Looks like there is a nondeterministic failure in the test: https://github.com/ruby/webrick/runs/894956154

Can you please add a pull request to fix that? Otherwise, I'll probably have to revert.

@wishdev wishdev deleted the shutdown_pipe_config branch July 28, 2020 20:28
@mame
Copy link
Member

mame commented Sep 24, 2020

I'm unsure if it is good to expose the internal. @akr introduced the mechanism of shutdown_pipe. @akr, what do you think?

@ko1
Copy link
Contributor

ko1 commented Sep 25, 2020

  1) Error:
TestWEBrickServer#test_shutdown_pipe:
IOError: stream closed in another thread
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `write'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `puts'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `block in test_shutdown_pipe'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:167:in `loop'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:167:in `test_shutdown_pipe'

on high load-average environments.

http://ci.rvm.jp/logfiles/brlog.trunk-no-theap.20200924-222101

@wishdev
Copy link
Contributor Author

wishdev commented Sep 25, 2020

  1) Error:
TestWEBrickServer#test_shutdown_pipe:
IOError: stream closed in another thread
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `write'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `puts'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:180:in `block in test_shutdown_pipe'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:167:in `loop'
    /tmp/ruby/v3/src/trunk-no-theap/test/webrick/test_server.rb:167:in `test_shutdown_pipe'

on high load-average environments.

http://ci.rvm.jp/logfiles/brlog.trunk-no-theap.20200924-222101

I'm going to discuss this on #54 because that's where the test code in question lives.

wishdev added a commit to wishdev/webrick that referenced this pull request Sep 25, 2020
As mentioned in ruby#44 and ruby#54 - there continues to be issues with regards
to the server startup with regards to this test. WEBrick::GenericServer#stop
swallows an IOError internally - so the fix here is to swallow the error
during the test and to retry.
@wishdev wishdev mentioned this pull request Sep 25, 2020
jeremyevans pushed a commit that referenced this pull request Sep 25, 2020
As mentioned in #44 and #54 - there continues to be issues with regards
to the server startup with regards to this test. WEBrick::GenericServer#stop
swallows an IOError internally - so the fix here is to swallow the error
during the test and to retry.
matzbot pushed a commit to ruby/ruby that referenced this pull request Nov 13, 2020
  Because the test for this change was still broken.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants