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

Start on a guide for the Executor & Load Interlock #27494

Merged
merged 2 commits into from Nov 16, 2017

Conversation

Projects
None yet
8 participants
@matthewd
Member

matthewd commented Dec 29, 2016

No description provided.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Dec 30, 2016

Contributor

Nice, thanks. The existing Autoloading and Reloading Constants might need a lookover, or at minimum a cross-link to this one.

Contributor

jrochkind commented Dec 30, 2016

Nice, thanks. The existing Autoloading and Reloading Constants might need a lookover, or at minimum a cross-link to this one.

end
```
NOTE: Concurrent Ruby uses a `ThreadPoolExecutor`, which it sometimes configures

This comment has been minimized.

@tomkadwill

tomkadwill Dec 30, 2016

Contributor

s/configures/configured/

@tomkadwill

tomkadwill Dec 30, 2016

Contributor

s/configures/configured/

This comment has been minimized.

@turboladen

turboladen Jan 5, 2017

"configures" seems ok here, no? Or does it not do this anymore?

@turboladen

turboladen Jan 5, 2017

"configures" seems ok here, no? Or does it not do this anymore?

Show outdated Hide outdated guides/source/threading_and_code_execution.md
Only long-running "top level" processes should invoke the Reloader, because if
it determines a reload is needed, it will block until all other threads have
completed and left any Executor block.

This comment has been minimized.

@tomkadwill

tomkadwill Dec 30, 2016

Contributor

s/block/blocks/

@tomkadwill

tomkadwill Dec 30, 2016

Contributor

s/block/blocks/

This comment has been minimized.

@jrochkind

jrochkind Jan 2, 2017

Contributor

suggest : "A Rails web app process takes care of invoking the Reloader for you under normal use." or similar.

@jrochkind

jrochkind Jan 2, 2017

Contributor

suggest : "A Rails web app process takes care of invoking the Reloader for you under normal use." or similar.

# call application code here
end
```

This comment has been minimized.

@jrochkind

jrochkind Dec 30, 2016

Contributor

I believe the Reloader is not re-entrant? Since you mentioned the executor is (thanks!), can you specify whether reloader is or is not intended to be?

@jrochkind

jrochkind Dec 30, 2016

Contributor

I believe the Reloader is not re-entrant? Since you mentioned the executor is (thanks!), can you specify whether reloader is or is not intended to be?

This comment has been minimized.

@matthewd

matthewd Dec 30, 2016

Member

It is.. I'm just not expecting that you'd use it in a place it matters:

The reloader (because, by design, it can change the application out from under you) is only really appropriate when you're in control, which implies you know how you got here / whether it's already been invoked.

The executor, OTOH, is fine to casually wrap around anything that strikes your fancy, "just in case" it isn't in play yet. Thus it seemed more important to address reentrancy, on the basis it's a concern a potential caller is likely to have.

Anyway, will clarify 👍🏻

@matthewd

matthewd Dec 30, 2016

Member

It is.. I'm just not expecting that you'd use it in a place it matters:

The reloader (because, by design, it can change the application out from under you) is only really appropriate when you're in control, which implies you know how you got here / whether it's already been invoked.

The executor, OTOH, is fine to casually wrap around anything that strikes your fancy, "just in case" it isn't in play yet. Thus it seemed more important to address reentrancy, on the basis it's a concern a potential caller is likely to have.

Anyway, will clarify 👍🏻

This comment has been minimized.

@jrochkind

jrochkind Jan 2, 2017

Contributor

awesome, this reply is helpful, recommend wrapping it into the guide more explicitly too.

@jrochkind

jrochkind Jan 2, 2017

Contributor

awesome, this reply is helpful, recommend wrapping it into the guide more explicitly too.

* enable and disable the Active Record query cache
* return acquired Active Record connections to the pool
* constrain internal cache lifetimes

This comment has been minimized.

@jrochkind

jrochkind Jan 2, 2017

Contributor

This Guide is great, thanks. Can you reference or link to where these callbacks are set up for default Rails app?

@jrochkind

jrochkind Jan 2, 2017

Contributor

This Guide is great, thanks. Can you reference or link to where these callbacks are set up for default Rails app?

This comment has been minimized.

@matthewd

matthewd Nov 16, 2017

Member

I don't think there's any precedent for that degree of source-code tourism in guides; the Autoloading and Reloading Constants guide is a very deep dive, but it still doesn't get that close to the implementation.

Apart from the risk of getting a reader bogged down in implementation detail (e.g. how our callbacks work, as opposed to how they're registered), I think the biggest worry with such links would be that they'll rot fairly quickly: these are couple-of-line instances in different places throughout the framework.

Like seriously there are just so many of them.

@matthewd

matthewd Nov 16, 2017

Member

I don't think there's any precedent for that degree of source-code tourism in guides; the Autoloading and Reloading Constants guide is a very deep dive, but it still doesn't get that close to the implementation.

Apart from the risk of getting a reader bogged down in implementation detail (e.g. how our callbacks work, as opposed to how they're registered), I think the biggest worry with such links would be that they'll rot fairly quickly: these are couple-of-line instances in different places throughout the framework.

Like seriously there are just so many of them.

Show outdated Hide outdated guides/source/documents.yaml
-
name: Threading and Code Execution in Rails
url: threading_and_code_execution.html
description: This suige describes the considerations needed and tools available when working directly with concurrency in a Rails application.

This comment has been minimized.

@rosenfeld

rosenfeld Jan 3, 2017

Contributor

minor typo here: suige => guide

@rosenfeld

rosenfeld Jan 3, 2017

Contributor

minor typo here: suige => guide

Show outdated Hide outdated guides/source/threading_and_code_execution.md
If it's impractical to physically wrap the application code in a block (for
example, the Rack API makes this problematic), you can also use the `run!` /
`complete!` pair.

This comment has been minimized.

@noahgibbs

noahgibbs Jan 5, 2017

Would be a great idea to have a small example here, something like:

Thread.new do
  begin
    Rails.application.executor.run!
      # your code here
  ensure
    Rails.application.executor.complete!
  end
end
@noahgibbs

noahgibbs Jan 5, 2017

Would be a great idea to have a small example here, something like:

Thread.new do
  begin
    Rails.application.executor.run!
      # your code here
  ensure
    Rails.application.executor.complete!
  end
end
Threaded Active Job adapters, including the built-in Async, will likewise
execute several jobs at the same time. Action Cable channels are managed this
way too.

This comment has been minimized.

@noahgibbs

noahgibbs Jan 5, 2017

This seems like as long as an Action Cable connection exists, reloading constants can't happen since the other threads will block (reasonable.) If that's true, is it worth mentioning it explicitly at some point, so that doesn't surprise people?

@noahgibbs

noahgibbs Jan 5, 2017

This seems like as long as an Action Cable connection exists, reloading constants can't happen since the other threads will block (reasonable.) If that's true, is it worth mentioning it explicitly at some point, so that doesn't surprise people?

This comment has been minimized.

@matthewd

matthewd Nov 16, 2017

Member

It's not true, because that would surprise people 😉

I've added a new section on Framework Behavior, and described what Action Cable does in there. The too-short version is that it [hopefully] behaves the way someone might expect before they stop and think about it.

@matthewd

matthewd Nov 16, 2017

Member

It's not true, because that would surprise people 😉

I've added a new section on Framework Behavior, and described what Action Cable does in there. The too-short version is that it [hopefully] behaves the way someone might expect before they stop and think about it.

@patrickbrownsync

This comment has been minimized.

Show comment
Hide comment
@patrickbrownsync

patrickbrownsync Jun 5, 2017

Should this include some information on using or not using ActiveRecord::Base.connection_pool.with_connection ?

Seems like Rails.application.executor.wrap may do this for you, so the previous recommendation to use with_connection might not be true anymore.

Should this include some information on using or not using ActiveRecord::Base.connection_pool.with_connection ?

Seems like Rails.application.executor.wrap may do this for you, so the previous recommendation to use with_connection might not be true anymore.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Nov 7, 2017

Contributor

Is this un-merged PR what we've got for any official documentation on this topic? @matthewd , any progress or plan to end up with released docs?

Contributor

jrochkind commented Nov 7, 2017

Is this un-merged PR what we've got for any official documentation on this topic? @matthewd , any progress or plan to end up with released docs?

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Nov 7, 2017

Contributor

This is super helpful.

I'd love it if it could explain how the necessity for these mechanisms is effected by Rails eager_load/autoload/cache_classes configuration -- do you need to wrap in an executor even in standard production configuration? Are there any performance considerations to doing so even if you don't need to?

Contributor

jrochkind commented Nov 7, 2017

This is super helpful.

I'd love it if it could explain how the necessity for these mechanisms is effected by Rails eager_load/autoload/cache_classes configuration -- do you need to wrap in an executor even in standard production configuration? Are there any performance considerations to doing so even if you don't need to?

@matthewd

Having allowed everyone's great feedback to age like a fine wine for nearly a year 😣, I think I've now addressed everything mentioned

* enable and disable the Active Record query cache
* return acquired Active Record connections to the pool
* constrain internal cache lifetimes

This comment has been minimized.

@matthewd

matthewd Nov 16, 2017

Member

I don't think there's any precedent for that degree of source-code tourism in guides; the Autoloading and Reloading Constants guide is a very deep dive, but it still doesn't get that close to the implementation.

Apart from the risk of getting a reader bogged down in implementation detail (e.g. how our callbacks work, as opposed to how they're registered), I think the biggest worry with such links would be that they'll rot fairly quickly: these are couple-of-line instances in different places throughout the framework.

Like seriously there are just so many of them.

@matthewd

matthewd Nov 16, 2017

Member

I don't think there's any precedent for that degree of source-code tourism in guides; the Autoloading and Reloading Constants guide is a very deep dive, but it still doesn't get that close to the implementation.

Apart from the risk of getting a reader bogged down in implementation detail (e.g. how our callbacks work, as opposed to how they're registered), I think the biggest worry with such links would be that they'll rot fairly quickly: these are couple-of-line instances in different places throughout the framework.

Like seriously there are just so many of them.

Threaded Active Job adapters, including the built-in Async, will likewise
execute several jobs at the same time. Action Cable channels are managed this
way too.

This comment has been minimized.

@matthewd

matthewd Nov 16, 2017

Member

It's not true, because that would surprise people 😉

I've added a new section on Framework Behavior, and described what Action Cable does in there. The too-short version is that it [hopefully] behaves the way someone might expect before they stop and think about it.

@matthewd

matthewd Nov 16, 2017

Member

It's not true, because that would surprise people 😉

I've added a new section on Framework Behavior, and described what Action Cable does in there. The too-short version is that it [hopefully] behaves the way someone might expect before they stop and think about it.

@matthewd matthewd merged commit 8bfa617 into rails:master Nov 16, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matthewd matthewd deleted the matthewd:executor-guide branch Dec 2, 2017

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