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

Initial Seperation of CLI and Server Launcher work #856

Merged
merged 17 commits into from Feb 6, 2016

Conversation

Projects
None yet
3 participants
@schneems
Contributor

schneems commented Jan 14, 2016

This is a WIP. This was the minimum I could do to get all tests to pass without changing any tests. Eventually I think we want all high level process controls to come from launcher, I also think we want another separate object that gets passed to Runner/Single/Cluster that will maintain a relationship with the Launcher. We could use this as the object that also gets exposed to the app like the Embeddable class we talked about earlier.

Moving forwards i'm planning to port out the CLI tests to only test that they are parsing the correct config and launching servers. I'll port all low level unit tests over to the launcher. Making this change we could either keep all the public methods in CLI that delegate to @launcher, I'm guessing not many people are using the internals of CLI and we can take them out. It's your call though.

Wanted to kick this over the fence and see if you had any strong reactions or feelings about this approach.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 14, 2016

Contributor
Contributor

schneems commented Jan 14, 2016

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Jan 14, 2016

Member

I like where this is going. Pull the code out of the CLI into the Launcher then having the CLI just use the Launcher means the code paths don't get stale.

Member

evanphx commented Jan 14, 2016

I like where this is going. Pull the code out of the CLI into the Launcher then having the CLI just use the Launcher means the code paths don't get stale.

@schneems schneems referenced this pull request Jan 21, 2016

Merged

Add Default Puma Config #23057

schneems added some commits Jan 14, 2016

Initial Seperation of CLI and Server Launcher work
This is a WIP. This was the minimum I could do to get all tests to pass without changing any tests. Eventually I think we want all high level process controls to come from launcher, I also think we want another separate object that gets passed to Runner/Single/Cluster that will maintain a relationship with the Launcher. We could use this as the object that also gets exposed to the app like the Embeddable class we talked about earlier. 

Moving forwards i'm planning to port out the CLI tests to only test that they are parsing the correct config and launching servers. I'll port all low level unit tests over to the launcher. Making this change we could either keep all the public methods in CLI that delegate to `@launcher`, I'm guessing not many people are using the internals of CLI and we can take them out. It's your call though.

Wanted to kick this over the fence and see if you had any strong reactions or feelings about this approach.
Move more logic out of CLI
Focus on removing @options. Delegate all actions to launcher.
Remove config from CLI
Don't store @cli_options in Launcher (there are too many "options" hashes and their roles are confusing, let's make them more obvious, the arguments passed to cli_options are parsed and turned into @options).
More explicit options
The "cli_options" aren't always going to be from a CLI, also this makes it explicit that they are an input, and therefore not cannonical. Switch to `launcher_args` because if I was writing this in Ruby 2.1 they would be kwargs, and the "options" word is confusing since there's enough arguments.
Fix Puma with `rails server`
The "default" thread in the handler was interpreted as canonical and took precedence over the `config/puma.rb` file. Fixed by using defaults already present in `configuration.rb` which is used by the Launcher.

We only advertise `Puma.cli_config` when puma is set via the cli. Not sure why but if `cli.rb` hasn't been loaded then we don't need to run that code.

Moving requires to Launcher so it can be called as a standalone file (otherwise we get require errors).
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 4, 2016

Contributor

Okay, I got all the tests to pass here. This was WAY more work than I was thinking originally. I was successful in extracting out a general purpose puma launching class Launcher and re-using that in the CLI and in the rack handler. As far as I know this is 100% API backwards compatible minus one thing:

The one backwards incompatible thing: The original Puma rack handler would yield to an instance of Puma::Server when called with a block. Since we're not creating a server manually anymore, i'm yielding to an instance of Launcher instead. I'm not sure how much people use this behavior of the rack handler, or really even why it exists (though It was handy for writing tests, I added an extra one for doing an integration style test with the rack handler). The Launcher does have access to a "runner" either an instance of Single or Cluster, each of them have a server object, but neither expose it. If you want I could do the work to expose it and then keep the rack handler block API the same, but it would increase the surface area of those to classes. So either we could not care about the API that maybe not many people use or I could increase some object surface area, it's up to you.

Let me know if you've got questions. I'm available to chat about this if you want, I think we're both in the Rails contributor basecamp chatroom if you want, or ping me in rubycentral chat. I could also jump on a hangout if you need more fidelity. I'll be around tomorrow and next week after Wednesday.

Contributor

schneems commented Feb 4, 2016

Okay, I got all the tests to pass here. This was WAY more work than I was thinking originally. I was successful in extracting out a general purpose puma launching class Launcher and re-using that in the CLI and in the rack handler. As far as I know this is 100% API backwards compatible minus one thing:

The one backwards incompatible thing: The original Puma rack handler would yield to an instance of Puma::Server when called with a block. Since we're not creating a server manually anymore, i'm yielding to an instance of Launcher instead. I'm not sure how much people use this behavior of the rack handler, or really even why it exists (though It was handy for writing tests, I added an extra one for doing an integration style test with the rack handler). The Launcher does have access to a "runner" either an instance of Single or Cluster, each of them have a server object, but neither expose it. If you want I could do the work to expose it and then keep the rack handler block API the same, but it would increase the surface area of those to classes. So either we could not care about the API that maybe not many people use or I could increase some object surface area, it's up to you.

Let me know if you've got questions. I'm available to chat about this if you want, I think we're both in the Rails contributor basecamp chatroom if you want, or ping me in rubycentral chat. I could also jump on a hangout if you need more fidelity. I'll be around tomorrow and next week after Wednesday.

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Feb 5, 2016

Member

Yay! I'm not concerned about the yield API. I doubt anyone used it, given
how strange and undocumented it was.

Given the compat layer you built in, if the tests pass I'm good with it!

I'll give it a further read tonight and ping you tomorrow with questions.
On Thu, Feb 4, 2016 at 3:15 PM Richard Schneeman notifications@github.com
wrote:

Okay, I got all the tests to pass here. This was WAY more work than I was
thinking originally. I was successful in extracting out a general purpose
puma launching class Launcher and re-using that in the CLI and in the
rack handler. As far as I know this is 100% API backwards compatible minus
one thing:

The one backwards incompatible thing: The original Puma rack handler would
yield to an instance of Puma::Server when called with a block. Since
we're not creating a server manually anymore, i'm yielding to an instance
of Launcher instead. I'm not sure how much people use this behavior of
the rack handler, or really even why it exists (though It was handy for
writing tests, I added an extra one for doing an integration style test
with the rack handler). The Launcher does have access to a "runner" either
an instance of Single or Cluster, each of them have a server object, but
neither expose it. If you want I could do the work to expose it and then
keep the rack handler block API the same, but it would increase the surface
area of those to classes. So either we could not care about the API that
maybe not many people use or I could increase some object surface area,
it's up to you.

Let me know if you've got questions. I'm available to chat about this if
you want, I think we're both in the Rails contributor basecamp chatroom if
you want, or ping me in rubycentral chat. I could also jump on a hangout if
you need more fidelity. I'll be around tomorrow and next week after
Wednesday.


Reply to this email directly or view it on GitHub
#856 (comment).

Member

evanphx commented Feb 5, 2016

Yay! I'm not concerned about the yield API. I doubt anyone used it, given
how strange and undocumented it was.

Given the compat layer you built in, if the tests pass I'm good with it!

I'll give it a further read tonight and ping you tomorrow with questions.
On Thu, Feb 4, 2016 at 3:15 PM Richard Schneeman notifications@github.com
wrote:

Okay, I got all the tests to pass here. This was WAY more work than I was
thinking originally. I was successful in extracting out a general purpose
puma launching class Launcher and re-using that in the CLI and in the
rack handler. As far as I know this is 100% API backwards compatible minus
one thing:

The one backwards incompatible thing: The original Puma rack handler would
yield to an instance of Puma::Server when called with a block. Since
we're not creating a server manually anymore, i'm yielding to an instance
of Launcher instead. I'm not sure how much people use this behavior of
the rack handler, or really even why it exists (though It was handy for
writing tests, I added an extra one for doing an integration style test
with the rack handler). The Launcher does have access to a "runner" either
an instance of Single or Cluster, each of them have a server object, but
neither expose it. If you want I could do the work to expose it and then
keep the rack handler block API the same, but it would increase the surface
area of those to classes. So either we could not care about the API that
maybe not many people use or I could increase some object surface area,
it's up to you.

Let me know if you've got questions. I'm available to chat about this if
you want, I think we're both in the Rails contributor basecamp chatroom if
you want, or ping me in rubycentral chat. I could also jump on a hangout if
you need more fidelity. I'll be around tomorrow and next week after
Wednesday.


Reply to this email directly or view it on GitHub
#856 (comment).

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 5, 2016

Contributor

Any questions or concerns? We could add a deprecation to the CLI methods if we want to decrease surface area in the future, but I don't think it's needed. I don't see any downside to keeping those delegate methods in there.

The other thing I wanted to do, but it is very ambitious is to not pass the Launcher object to Single and Cluster and instead pass an interchange object. Ideally it would have a smaller API and if we wanted to expose some kind of intermediate control layer directly to the app I would rather we did it without going directly through the Launcher object. I spent a little time investigating that change, and it is pretty large. The classes that inherit from Runner directly use the @cli object pretty liberally, and they also directly use the @cli.options hash. There wasn't an immediate benefit from going down that road so I stopped.

Contributor

schneems commented Feb 5, 2016

Any questions or concerns? We could add a deprecation to the CLI methods if we want to decrease surface area in the future, but I don't think it's needed. I don't see any downside to keeping those delegate methods in there.

The other thing I wanted to do, but it is very ambitious is to not pass the Launcher object to Single and Cluster and instead pass an interchange object. Ideally it would have a smaller API and if we wanted to expose some kind of intermediate control layer directly to the app I would rather we did it without going directly through the Launcher object. I spent a little time investigating that change, and it is pretty large. The classes that inherit from Runner directly use the @cli object pretty liberally, and they also directly use the @cli.options hash. There wasn't an immediate benefit from going down that road so I stopped.

@evanphx

This comment has been minimized.

Show comment
Hide comment
@evanphx

evanphx Feb 6, 2016

Member

Looks great! I'm going to go ahead and merge it. I'm going to probably add a few helpers as some further documentation to Launcher.

This weekend I'm going to be adding a plugin system to puma as well. After that, I'm going to go ahead and release all this work as 3.0!

Member

evanphx commented Feb 6, 2016

Looks great! I'm going to go ahead and merge it. I'm going to probably add a few helpers as some further documentation to Launcher.

This weekend I'm going to be adding a plugin system to puma as well. After that, I'm going to go ahead and release all this work as 3.0!

evanphx added a commit that referenced this pull request Feb 6, 2016

Merge pull request #856 from schneems/schneems/launcher
Initial Seperation of CLI and Server Launcher work

@evanphx evanphx merged commit 5473396 into puma:master Feb 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 6, 2016

Contributor

🎉🎉🎉 let me know when 3.0 goes out and I can update the Gemfile that rails generates to use a compatible version of Puma. Thanks for your help!

Contributor

schneems commented Feb 6, 2016

🎉🎉🎉 let me know when 3.0 goes out and I can update the Gemfile that rails generates to use a compatible version of Puma. Thanks for your help!

schneems added a commit to schneems/rails that referenced this pull request Feb 26, 2016

[close #23681] Use puma 3.0.0+
Puma 3.0 and up introduced compatibility to read from `config/puma.rb` when booting from the command `$ rails server`puma/puma#856.

chashmeetsingh added a commit to chashmeetsingh/rails that referenced this pull request Feb 26, 2016

Add `issue_template.md` and `pull_request_template.md`
This appears to be a new feature of GitHub.

See these links for more details:
- dear-github/dear-github#125
- https://github.com/owncloud/core/issues/new
- https://github.com/blog/2111-issue-and-pull-request-templates

[ci skip]

The async.callback call should live with the hijack

If we're deferring one, we should defer the other too.

💅

[skip ci]

fix typo in pull_request_template [ci skip]

- Changed Debugging Rails Applications doc's logger introduction section. Changed location for specifying logger.
[Prajakta, thiagoaugusto]

[close #23681] Use puma 3.0.0+

Puma 3.0 and up introduced compatibility to read from `config/puma.rb` when booting from the command `$ rails server`puma/puma#856.

Use redis_connector to create redis connections for both subscriptions and broadcasts

Added log "Rendering ...", when starting to render a template, to log that we have started to render something, at the very beginning.
This helps to easily identify queries from controller vs views

Fixes #23710

Update to use Subscriber#start instead

We don't need to instrument another event as
`ActiveSupport::LogSubscriber` already tracks when the instrumentation
starts.

Close #23717

Add CHANGELOG entry for "Rendering ..." logging
@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Mar 3, 2016

Contributor

Hi,

I'm debugging #919. I think it's related to this change. Now that puma loads config/puma.rb by default, I'm having errors using my application code in hooks like before_fork or on_worker_boot. How do you make them work in Rails?

Thanks!

PD: I made a simple application that shows the error: https://github.com/frodsan/shotgun-puma-bug.
You can check the config/puma.rb file.

Contributor

frodsan commented Mar 3, 2016

Hi,

I'm debugging #919. I think it's related to this change. Now that puma loads config/puma.rb by default, I'm having errors using my application code in hooks like before_fork or on_worker_boot. How do you make them work in Rails?

Thanks!

PD: I made a simple application that shows the error: https://github.com/frodsan/shotgun-puma-bug.
You can check the config/puma.rb file.

@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Mar 3, 2016

Contributor

I checked the issue again. It works if I use puma:

$ puma
[3933] Puma starting in cluster mode...
[3933] * Version 3.0.2 (ruby 2.3.0-p0), codename: Plethora of Penguin Pinatas
[3933] * Min threads: 5, max threads: 5
[3933] * Environment: development
[3933] * Process workers: 2
[3933] * Preloading application
[3933] * Listening on tcp://0.0.0.0:

It looks like the problem is in shotgun :-(

Contributor

frodsan commented Mar 3, 2016

I checked the issue again. It works if I use puma:

$ puma
[3933] Puma starting in cluster mode...
[3933] * Version 3.0.2 (ruby 2.3.0-p0), codename: Plethora of Penguin Pinatas
[3933] * Min threads: 5, max threads: 5
[3933] * Environment: development
[3933] * Process workers: 2
[3933] * Preloading application
[3933] * Listening on tcp://0.0.0.0:

It looks like the problem is in shotgun :-(

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Mar 3, 2016

Contributor

Hey, thanks for the report can you open up a separate issue so we can track this? What do you think the problem is with shotgun? That constant should be defined when you require the puma rack handler.

Contributor

schneems commented Mar 3, 2016

Hey, thanks for the report can you open up a separate issue so we can track this? What do you think the problem is with shotgun? That constant should be defined when you require the puma rack handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment