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

Publish AS::Executor and AS::Reloader APIs #23807

Merged
merged 4 commits into from Mar 1, 2016

Conversation

Projects
None yet
@matthewd
Member

matthewd commented Feb 22, 2016

These should allow external code to run blocks of user code to do "work", at a similar unit size to a web request, without needing to get intimate with ActionDispatch.

The theory is that interested callers (Sidekiq, ActionCable) can just do:

Rails.application.reloader.wrap do
  # run some user code
end

.. and we'll take care of the interlock, code reloading, returning AR connections to the pool, and anything else that might be relevant.

@matthewd matthewd self-assigned this Feb 22, 2016

@matthewd matthewd added this to the 5.0.0 milestone Feb 22, 2016

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Feb 22, 2016

Member

😱💚

Member

vipulnsward commented Feb 22, 2016

😱💚

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 22, 2016

Member

This is really promising. I like the API and the implementation is simple. 👍

Member

rafaelfranca commented Feb 22, 2016

This is really promising. I like the API and the implementation is simple. 👍

@matthewd matthewd changed the title from [WIP] Publish AS::Executor and AS::Reloader APIs to Publish AS::Executor and AS::Reloader APIs Feb 27, 2016

<%- unless options[:skip_action_cable] -%>
# Action Cable requires that all classes are loaded in advance
Rails.application.eager_load!

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 29, 2016

Member

Is this not needed anymore?

@rafaelfranca

rafaelfranca Feb 29, 2016

Member

Is this not needed anymore?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 29, 2016

Member

Ah. it was because Action Cable, yeah, it is not needed.

@rafaelfranca

rafaelfranca Feb 29, 2016

Member

Ah. it was because Action Cable, yeah, it is not needed.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 29, 2016

Member

Love this ❤️

Member

kaspth commented Feb 29, 2016

Love this ❤️

matthewd added some commits Feb 22, 2016

Publish AS::Executor and AS::Reloader APIs
These should allow external code to run blocks of user code to do
"work", at a similar unit size to a web request, without needing to get
intimate with ActionDipatch.

matthewd added a commit that referenced this pull request Mar 1, 2016

Merge pull request #23807 from matthewd/executor
Publish AS::Executor and AS::Reloader APIs

@matthewd matthewd merged commit 541e4ab into rails:master Mar 1, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Mar 1, 2016

Member

🤘

Member

jeremy commented Mar 1, 2016

🤘

@mikhailov

This comment has been minimized.

Show comment
Hide comment
@mikhailov

mikhailov Mar 2, 2016

👍 Since we have an option to reload user code what you think to enhance the functionality to avoid unnecessary reload for static assets (idea came from #23518)

mikhailov commented Mar 2, 2016

👍 Since we have an option to reload user code what you think to enhance the functionality to avoid unnecessary reload for static assets (idea came from #23518)

spastorino pushed a commit that referenced this pull request Mar 2, 2016

Jorge Bejar and Santiago Pastorino
Do not run app.executor callbacks in integration tests
This reverts changes made to integration tests in PR #23807.
The issue happens when using capybara with a driver that needs to start a
server in a separate thread like (poltergeist, selenium, etc).
Both threads the capybara server one and the test thread end running
syncronize over the interlock.
@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino Mar 2, 2016

Member

The integration.rb part of the changes introduces an issue when using capybara with a driver that needs to start a server in a separate thread like poltergeist, selenium, etc.

Both threads, the test thread and the capybara server one run synchronize over the interlock so the server can't execute when receives the request because it's locked.

I don't know what was the intention of the changes in integration.rb were but doesn't seem to have sense, given that all it adds is this lock and QueryCache which doesn't make much sense per integration test as it's working with this changes.

Fixed here cf075d9

Member

spastorino commented Mar 2, 2016

The integration.rb part of the changes introduces an issue when using capybara with a driver that needs to start a server in a separate thread like poltergeist, selenium, etc.

Both threads, the test thread and the capybara server one run synchronize over the interlock so the server can't execute when receives the request because it's locked.

I don't know what was the intention of the changes in integration.rb were but doesn't seem to have sense, given that all it adds is this lock and QueryCache which doesn't make much sense per integration test as it's working with this changes.

Fixed here cf075d9

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Mar 5, 2016

Contributor

Would there be any benefit to using call instead of wrap as the method name? More idiomatic or reusable?

Contributor

mperham commented Mar 5, 2016

Would there be any benefit to using call instead of wrap as the method name? More idiomatic or reusable?

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Apr 10, 2016

Member

This changed the QueryCache to be an executor instead of a middleware.
Before, to disable QueryCache in a rails app, I could just remove the middleware. (config.middleware.delete "ActiveRecord::QueryCache"). How can I disable it now? should we add some way to?

Member

arthurnn commented Apr 10, 2016

This changed the QueryCache to be an executor instead of a middleware.
Before, to disable QueryCache in a rails app, I could just remove the middleware. (config.middleware.delete "ActiveRecord::QueryCache"). How can I disable it now? should we add some way to?

@mrrusof

This comment has been minimized.

Show comment
Hide comment
@mrrusof

mrrusof Apr 20, 2016

I see you touched ActiveRecord::ConnectionAdapters::ConnectionManagement. I'm in the process of figuring out how to reconcile Sinatra+Thin+ActiveRecord. The problem is that ConnectionManagement in ActiveRecord 4.2.6 does not return connections to the pool when used by Thin 1.6.4. ConnectionManagement returns to the pool those connections that are active and belong to the current thread. For each request, Thin connects to the database in a thread and applies ConnectionManagement in another.

Does this pull request resolve the problem? If not, what is "the right way" to approach the problem?

Related questions in stackoverflow:

mrrusof commented Apr 20, 2016

I see you touched ActiveRecord::ConnectionAdapters::ConnectionManagement. I'm in the process of figuring out how to reconcile Sinatra+Thin+ActiveRecord. The problem is that ConnectionManagement in ActiveRecord 4.2.6 does not return connections to the pool when used by Thin 1.6.4. ConnectionManagement returns to the pool those connections that are active and belong to the current thread. For each request, Thin connects to the database in a thread and applies ConnectionManagement in another.

Does this pull request resolve the problem? If not, what is "the right way" to approach the problem?

Related questions in stackoverflow:

willnet added a commit to willnet/rails that referenced this pull request May 1, 2016

Replace ActionDispatch::LoadInterlock with ActionDispatch::Executor i…
…n guides [ci skip]

Guides should be updated because ActionDispatch::LoadInterlock was replaced with
ActionDispatch::Executor at #23807.

jeremy added a commit that referenced this pull request May 1, 2016

Replace ActionDispatch::LoadInterlock with ActionDispatch::Executor i…
…n guides [ci skip]

Guides should be updated because ActionDispatch::LoadInterlock was replaced with
ActionDispatch::Executor at #23807.

Neodelf added a commit to Neodelf/rails that referenced this pull request May 5, 2016

Replace ActionDispatch::LoadInterlock with ActionDispatch::Executor i…
…n guides [ci skip]

Guides should be updated because ActionDispatch::LoadInterlock was replaced with
ActionDispatch::Executor at #23807.
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 30, 2016

Member

Does this pull request resolve the problem? If not, what is "the right way" to approach the problem?

@mrrusof I dont think it does. There is #24608 for instance to deal with connection pool, etc. if thats what you are asking.

Member

arthurnn commented May 30, 2016

Does this pull request resolve the problem? If not, what is "the right way" to approach the problem?

@mrrusof I dont think it does. There is #24608 for instance to deal with connection pool, etc. if thats what you are asking.

@mrrusof

This comment has been minimized.

Show comment
Hide comment
@mrrusof

mrrusof May 30, 2016

@arthurnn Well, I created an issue in Thin and the author resolved the problem by finishing requests in the same thread that starts them.

mrrusof commented May 30, 2016

@arthurnn Well, I created an issue in Thin and the author resolved the problem by finishing requests in the same thread that starts them.

@flyfy1

This comment has been minimized.

Show comment
Hide comment
@flyfy1

flyfy1 Dec 9, 2016

Why the prepare method is to run after the :run, but not before?

Why the prepare method is to run after the :run, but not before?

@matthewd matthewd deleted the matthewd:executor branch Jun 11, 2017

joecorcoran added a commit to travis-ci/travis-api that referenced this pull request Feb 20, 2018

joecorcoran added a commit to travis-ci/travis-api that referenced this pull request Feb 20, 2018

joecorcoran added a commit to travis-ci/travis-api that referenced this pull request Feb 21, 2018

joecorcoran added a commit to travis-ci/travis-api that referenced this pull request Feb 21, 2018

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