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

worker_timeout, worker_boot_timeout, worker_shutdown_timeout fix integer convert #1450

Merged
merged 5 commits into from
Mar 19, 2018

Conversation

mitto
Copy link
Contributor

@mitto mitto commented Nov 9, 2017

When passing a character string of seconds to worker_timeout etc.,
In order to prevent the occurrence of the following error, we convert it to an integer like other options.

/Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:89:in `>': comparison of Float with String failed (ArgumentError)
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:89:in `ping_timeout?'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:179:in `block in check_workers'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:177:in `each'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:177:in `check_workers'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cluster.rb:471:in `run'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/launcher.rb:183:in `run'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/lib/puma/cli.rb:77:in `run'
        from /Users/mitto/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/puma-3.10.0/bin/puma-wild:31:in `<main>'

@nateberkopec
Copy link
Member

Fair enough. Needs a test.

@mitto
Copy link
Contributor Author

mitto commented Nov 14, 2017

As I wrote a test case, I found that persistent_timeout and first_data_timeout have similar problems, so I tried fix it.

@olleolleolle
Copy link
Contributor

The test is quite clear.

The build:

  • MRI failure looks like a flake failure
  • JRuby failure could perhaps be a flaky test? Can we retry this rebased on master?

@mitto mitto force-pushed the fix-missing-integer-convert branch from 1295c36 to b7f4c97 Compare December 25, 2017 13:43
@mitto
Copy link
Contributor Author

mitto commented Dec 25, 2017

@olleolleolle thank you for review.
I tried rebase the branch.
but, rbx-3 and 2.1 build failed.. 😢
It seems that the test case irrelevant to this pull request modification fails, but what should I do?

@olleolleolle
Copy link
Contributor

olleolleolle commented Dec 25, 2017

The issue is a current misbehavior in RubyGems + Bundler - a Bundler comes prepackaged with RubyGems. It can turn out that your installed bundle executable won't be found by Ruby.

Current thing creating issues: The integration tests want to run the following shell command:

  def server(argv)
    base = "#{Gem.ruby} -Ilib bin/puma"
    base.prepend("bundle exec ") if defined?(Bundler)
    cmd = "#{base} -b tcp://127.0.0.1:#{@tcp_port} #{argv}"
    @server = IO.popen(cmd, "r")

    wait_for_server_to_boot

    @server
  end

this includes a bundle exec.

I worked around this in some private code, today, by not shelling out to bundle exec, but that's not feasible here, I guess. TL;DR: The following won't help here.

In that case, I did two things:

  1. I had before_install with redundant RubyGems upgrades:
before_install:
  - gem update --system
  - gem install bundler
  1. Move out rvm elements to an explicit matrix description, to avoid shelling out to bundle exec inside the Ruby code (I moved the command that needed bundle exec in front of it out to the before_script step)
matrix:
  include:
    - rvm: 2.4.2
      before_script:
        - bundle exec rake db:create db:migrate
        - bundle exec sequel postgresql://localhost/test_sessions -m migrations
    - rvm: jruby-9.1.13.0
      before_script:
        - bundle exec rake db:create db:migrate
        - bundle exec sequel jdbc:postgresql://localhost/test_sessions -m migrations

Workarounds which include downgrading:

  • try installing a specific version of Bundler? Example: gem install bundler -v1.15.4
  • try installing a specific version of RubyGems? Example: gem update --system 2.6.14

@olleolleolle
Copy link
Contributor

olleolleolle commented Dec 25, 2017

Oh, 2.7.4 was just released!

Related: rubocop/rubocop@94174b1

@mitto mitto force-pushed the fix-missing-integer-convert branch 4 times, most recently from de02dda to c6c7c65 Compare December 26, 2017 11:39
@mitto mitto force-pushed the fix-missing-integer-convert branch from c6c7c65 to 2e16719 Compare December 26, 2017 12:41
@mitto
Copy link
Contributor Author

mitto commented Dec 26, 2017

@olleolleolle I tried modifying the build step several times and the build came to be successful but how about this?

@nateberkopec nateberkopec merged commit cf15db2 into puma:master Mar 19, 2018
@nateberkopec
Copy link
Member

Really high quality PR, thanks very much for this.

@mitto mitto deleted the fix-missing-integer-convert branch April 6, 2018 16:33
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.

None yet

3 participants