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

Consolidate option handling in Server, Server small refactors, doc changes #2373

Merged
merged 9 commits into from Sep 23, 2020

Conversation

@MSP-Greg
Copy link
Contributor

@MSP-Greg MSP-Greg commented Sep 19, 2020

Description

In normal use, Server is passed an options hash in its initialize method. Some code is outside of Server, using attr_accessors, some of which may have been created for CI, etc. They also display in docs, which isn't needed.

  1. server.rb - consolidate option handling, remove attr_accessor statements, change other lib files as needed.

  2. server.rb - Server had a method defined in class << self (tcp_cork_supported?), and also a method defined as self.current. Move current into class << self block.

  3. runner.rb - remove development? & test?, which are only used to set a value based on options in Server.

  4. Change test files to use Server#instance_variable_get

  5. Add YARD # @!attribute tags to methods that are actually attributes (no parameters, getter, setter, or predicate)

For those interested in seeing Puma docs:
current gem https://puma.io/puma
master https://msp-greg.github.io/puma/

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • [?] My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
@dentarg
Copy link
Member

@dentarg dentarg commented Sep 20, 2020

  1. remove attr_accessor statements

Could be considered breaking changes if someone is working programmatically against Puma::Server?

@MSP-Greg
Copy link
Contributor Author

@MSP-Greg MSP-Greg commented Sep 20, 2020

@dentarg

Could be considered breaking changes

Yes, that is true. But, given that Puma is essentially a CLI app, I don't think the idea of 'what is the public API' has been given much attention. Puma is not a young app, and over time, proper encapsulation of objects often leaks.

From my perspective, when I see attr_accessor or attr_writer, I wonder why write access is given. In the case of Server, there really isn't any reason for write access to exist.

I suppose I can revert all the attribute changes, but they just clutter up the real methods of the class. Maybe we should implement 'deprecated' warnings? I don't know.

I think that the more the API is cleaned up, the easier it is for new contributors to understand how Puma works...

@nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented Sep 22, 2020

I'm OK with removing write access where that write access didn't do anything (e.g. we never recheck the value after server start, so writing to the variable is a no-op).

I don't think we can remove reads without a deprecation policy anymore.

@MSP-Greg
Copy link
Contributor Author

@MSP-Greg MSP-Greg commented Sep 22, 2020

I did check what was passed to the various hooks and the plugin, I'll add back in reader code.

EDIT: Rebased and added attr_reader statements...

MSP-Greg added 9 commits Sep 19, 2020
…iable
@MSP-Greg MSP-Greg force-pushed the MSP-Greg:min-max branch from 07a68fa to 5114a34 Sep 22, 2020
@MSP-Greg
Copy link
Contributor Author

@MSP-Greg MSP-Greg commented Sep 23, 2020

One issue. In server.rb:83, I changed @max_threads from 16 to (Puma.mri? ? 5 : 16), which aligns it with the new default.

@nateberkopec nateberkopec merged commit bbbdfb8 into puma:master Sep 23, 2020
33 checks passed
33 checks passed
ubuntu-20.04 2.5
Details
build
Details
ubuntu-20.04 2.7
Details
ubuntu-20.04 2.7
Details
ubuntu-20.04 jruby
Details
ubuntu-20.04 head
Details
windows-2019 2.7
Details
ubuntu-18.04 2.2
Details
ubuntu-18.04 2.3
Details
ubuntu-18.04 2.4
Details
ubuntu-18.04 2.5
Details
ubuntu-18.04 2.6
Details
ubuntu-18.04 2.7
Details
ubuntu-18.04 head
Details
ubuntu-18.04 jruby
Details
ubuntu-18.04 jruby-head ubuntu-18.04 jruby-head
Details
ubuntu-18.04 truffleruby-head
Details
macos-10.15 2.2
Details
macos-10.15 2.3
Details
macos-10.15 2.4
Details
macos-10.15 2.5
Details
macos-10.15 2.6
Details
macos-10.15 2.7
Details
macos-10.15 head
Details
macos-10.15 jruby
Details
macos-10.15 truffleruby-head
Details
windows-2019 2.2
Details
windows-2019 2.3
Details
windows-2019 2.4
Details
windows-2019 2.5
Details
windows-2019 2.6
Details
windows-2019 2.7
Details
windows-2019 mingw
Details
@MSP-Greg MSP-Greg deleted the MSP-Greg:min-max branch Sep 23, 2020
nateberkopec added a commit that referenced this pull request Sep 28, 2020
…, doc changes (#2373)"

This reverts commit bbbdfb8.
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 28, 2020
…mall refactors, doc changes (puma#2373)"

Add attr_writer's to maintain API, mark deprecated in v6.0.0.
@dentarg
Copy link
Member

@dentarg dentarg commented Sep 28, 2020

  1. remove attr_accessor statements

Could be considered breaking changes if someone is working programmatically against Puma::Server?

Sorry, but this time I'm just gonna: #2388 😉

@MSP-Greg
Copy link
Contributor Author

@MSP-Greg MSP-Greg commented Sep 28, 2020

I've got an update that adds writers to this commit and marks them deprecated. Haven't pushed yet. It was mostly to clean up the following:

class Server
  attr_accessor :min_threads, :max_threads
  
  def initialize(app, events=Events.stdio, options={})
    # code
  end
end

class Runner
  def start_server
    server = Server.new(app, events, opts)
    server.min_threads = opts[:min_threads]
    server.max_threads = opts[:max_threads]
    # code
  end
end

So, we're passing opts to Server.new, then setting two values in start_server immediately after using opts. Breaks encapsulation, and adds unneeded code to start_server...

@MSP-Greg MSP-Greg restored the MSP-Greg:min-max branch Sep 28, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 29, 2020
…anges

Taken from PR puma#2373, added attr_writer's to maintain API, mark deprecated in v6.0.0.
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 29, 2020
Taken from PR puma#2373, added attr_writer's to maintain API, mark deprecated in v6.0.0.
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 29, 2020
Taken from PR puma#2373, added attr_writer's to maintain API, mark deprecated in v6.0.0.
@MSP-Greg MSP-Greg deleted the MSP-Greg:min-max branch Sep 29, 2020
nateberkopec pushed a commit that referenced this pull request Sep 29, 2020
…2389)

* Server - Consolidate option handling, small refactors, doc changes

Taken from PR #2373, added attr_writer's to maintain API, mark deprecated in v6.0.0.

* test_puma_server.rb - small timing fix for intermittent failures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.