Remove a potentially redundant statement #234

Closed
carsomyr opened this Issue Jul 3, 2012 · 13 comments

Comments

Projects
None yet
3 participants
@carsomyr
Contributor

carsomyr commented Jul 3, 2012

When making significant modifications to the Capistrano code, I came across the following statement in the Capistrano::Configuration::Execution module as part of the transaction/rollback mechanism.

self.rollback_requests = nil if Thread.main == Thread.current

Is the conditional check really necessary? I don't see Thread.main referenced anywhere else. See a512669 for details.

@leehambley

This comment has been minimized.

Show comment Hide comment
@leehambley

leehambley Jul 4, 2012

Owner

I can't speak about this @carsomyr not sure what it refers to, I've never worked on this part of the code.

What modifications are you making to the cap code?

Owner

leehambley commented Jul 4, 2012

I can't speak about this @carsomyr not sure what it refers to, I've never worked on this part of the code.

What modifications are you making to the cap code?

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 4, 2012

Contributor

I'm going to contact @roidrage to see what he intended with commit 60c155b. I think it's highly likely that the conditional is redundant because it doesn't appear with an earlier, analogous assignment.

As for my modifications, I intend to introduce the concept of "facets", namespace components that modify the execution environment of the task they're declared with. I believe that this will solve the ad-hoc way in which the multistage and multiproject extensions have been implemented. As an example, declaring my_project:my_stage:deploy:setup would execute the project and stage facets which will intelligently configure the environment so that the setup task will execute for project my_project and stage my_stage. The work is ongoing here. I'm going to file a formal pull request for consideration when the feature is more complete.

Contributor

carsomyr commented Jul 4, 2012

I'm going to contact @roidrage to see what he intended with commit 60c155b. I think it's highly likely that the conditional is redundant because it doesn't appear with an earlier, analogous assignment.

As for my modifications, I intend to introduce the concept of "facets", namespace components that modify the execution environment of the task they're declared with. I believe that this will solve the ad-hoc way in which the multistage and multiproject extensions have been implemented. As an example, declaring my_project:my_stage:deploy:setup would execute the project and stage facets which will intelligently configure the environment so that the setup task will execute for project my_project and stage my_stage. The work is ongoing here. I'm going to file a formal pull request for consideration when the feature is more complete.

@leehambley

This comment has been minimized.

Show comment Hide comment
@leehambley

leehambley Jul 4, 2012

Owner

As for my modifications, I intend to introduce the concept of "facets", namespace components that modify the execution environment of the task they're declared with. I believe that this will solve the ad-hoc way in which the multistage and multiproject extensions have been implemented. As an example, declaring my_project:my_stage:deploy:setup would execute the project and stage facets which will intelligently configure the environment so that the setup task will execute for project my_project and stage my_stage.

I'm not sure I understand the need for that, and I'm not sure I'll pull it if it's too much of a change, I announced on the mailing list that I'm not working on Capistrano anymore until perhaps the end of the year. (and of course that means that I can't shepherd in massive changes)

The work is ongoing here. I'm going to file a formal pull request for consideration when the feature is more complete.

Please do, and don't be disheartened if I don't accept it, I'm trying to reduce the number of things that Capistrano does in the long run, it is already a horribly designed tool that is misused by too many people.

Owner

leehambley commented Jul 4, 2012

As for my modifications, I intend to introduce the concept of "facets", namespace components that modify the execution environment of the task they're declared with. I believe that this will solve the ad-hoc way in which the multistage and multiproject extensions have been implemented. As an example, declaring my_project:my_stage:deploy:setup would execute the project and stage facets which will intelligently configure the environment so that the setup task will execute for project my_project and stage my_stage.

I'm not sure I understand the need for that, and I'm not sure I'll pull it if it's too much of a change, I announced on the mailing list that I'm not working on Capistrano anymore until perhaps the end of the year. (and of course that means that I can't shepherd in massive changes)

The work is ongoing here. I'm going to file a formal pull request for consideration when the feature is more complete.

Please do, and don't be disheartened if I don't accept it, I'm trying to reduce the number of things that Capistrano does in the long run, it is already a horribly designed tool that is misused by too many people.

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 5, 2012

Contributor

I talked to @roidrage about the redundant statement, and here's his reply.

I had to go through the details and look in the far back of my brain, but I couldn't find or remember a compelling reason why it checks for the main thread in that piece of code. On the other hand, there may have been a reason I've put it there, but I can't for the life of me remember what it was, sorry :(

On my end of things, I reran the unit tests and, not surprisingly, nothing broke. I am still in favor of removing the statement for the following reasons:

  1. The transactions and rollback feature is single threaded. That makes the Thread.current == Thread.main statement redundant. It's something that could cause headaches later as said code actually has an effect under concurrent execution.
  2. Transactions begin and end on the same thread, which means that if there's there's an earlier assignment to the same thread-local variable, it should have the conditional too.

If you feel that it's appropriate to make the change, please go ahead and do so and close the issue.

Contributor

carsomyr commented Jul 5, 2012

I talked to @roidrage about the redundant statement, and here's his reply.

I had to go through the details and look in the far back of my brain, but I couldn't find or remember a compelling reason why it checks for the main thread in that piece of code. On the other hand, there may have been a reason I've put it there, but I can't for the life of me remember what it was, sorry :(

On my end of things, I reran the unit tests and, not surprisingly, nothing broke. I am still in favor of removing the statement for the following reasons:

  1. The transactions and rollback feature is single threaded. That makes the Thread.current == Thread.main statement redundant. It's something that could cause headaches later as said code actually has an effect under concurrent execution.
  2. Transactions begin and end on the same thread, which means that if there's there's an earlier assignment to the same thread-local variable, it should have the conditional too.

If you feel that it's appropriate to make the change, please go ahead and do so and close the issue.

@leehambley

This comment has been minimized.

Show comment Hide comment
@leehambley

leehambley Jul 5, 2012

Owner

I agree it's probably redundant.

I'll leave the issue open and we'll re-visit this when you send your PR.

Thanks (/cc @roidrage thanks!)

Owner

leehambley commented Jul 5, 2012

I agree it's probably redundant.

I'll leave the issue open and we'll re-visit this when you send your PR.

Thanks (/cc @roidrage thanks!)

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 5, 2012

Contributor

In fact, @roidrage pointed me to his parallel execution extension. Will have a definitive answer soon. As for the PR, you don't mean the PR for the unrelated, large feature I am implementing, right?

Contributor

carsomyr commented Jul 5, 2012

In fact, @roidrage pointed me to his parallel execution extension. Will have a definitive answer soon. As for the PR, you don't mean the PR for the unrelated, large feature I am implementing, right?

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 5, 2012

Contributor

Ok, I got to the bottom of this. @roidrage can correct me if my reconstruction is wrong. Three years ago, there was an attempt to introduce [https://github.com/roidrage/cap-ext-parallelize](parallel task processing) to Capistrano. As part of that effort, parts of the transaction/rollback system were changed in the Capistrano mainline development to use thread-local constructs. In particular (and this is the important bit), rollback semantics were defined such that only the main thread rolled back failed transactions; background threads would defer their rollbacks. Hence, we see the conditional statement if Thread.current == Thread.main.

While well-intentioned, the parallel extension dabbles with concurrency, and concurrency is hard to get right. In particular, I observed this behavior running with an unmodified Capistrano and cap-ext-parallelize.

The Capfile:


require "cap_ext_parallelize"

desc "Raise a deliberate error"
task :raise_error do
  raise "Deliberate error"
end

desc "Conduct a transaction"
task :transact1 do
  transaction do
    on_rollback do
      puts "rollback 1"
    end
    transact2
  end
end

desc "Conduct a transaction"
task :transact2 do
  transaction do
    on_rollback do
      puts "rollback 2"
    end
    raise_error
  end
end

parallelize(1) do |session|
  session.run do
    transact1
  end
end

The output:


 ** Running 1 threads in chunks of 1
 ** Running batch number 1
 ** Running block in background thread
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1
*** Subthread failed: Deliberate error
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1
Capfile:5:in `block in load': Deliberate error (RuntimeError)

Notice the attempt to rollback twice, once in the background thread and once in the main thread. To compound matters, I had to modify two places to get the thing to rollback once in the main thread:

  1. Delete the entire Capistrano::Configuration::Execution module belonging to the cap-ext-parallelize project.
  2. Add in the if Thread.current == Thread.main conditional to produce the line
self.rollback_requests = [] if Thread.current == Thread.main

in lib/capistrano/configuration/execution.rb.

Making the above changes results in the correct output:


 ** Running 1 threads in chunks of 1
 ** Running batch number 1
 ** Running block in background thread
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
 ** transaction: start
  * executing `raise_error'
*** Subthread failed: Deliberate error
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1

Since both Capistrano itself and cap-ext-parallelize extension need some fixing, I would recommend reverting the code that supports "main thread rolls back" semantics and defer to @roidrage for patching the extension. Again, commit ea155dd doesn't affect Capistrano's behavior sans extension; all of those constructs work in support of the extension.

Contributor

carsomyr commented Jul 5, 2012

Ok, I got to the bottom of this. @roidrage can correct me if my reconstruction is wrong. Three years ago, there was an attempt to introduce [https://github.com/roidrage/cap-ext-parallelize](parallel task processing) to Capistrano. As part of that effort, parts of the transaction/rollback system were changed in the Capistrano mainline development to use thread-local constructs. In particular (and this is the important bit), rollback semantics were defined such that only the main thread rolled back failed transactions; background threads would defer their rollbacks. Hence, we see the conditional statement if Thread.current == Thread.main.

While well-intentioned, the parallel extension dabbles with concurrency, and concurrency is hard to get right. In particular, I observed this behavior running with an unmodified Capistrano and cap-ext-parallelize.

The Capfile:


require "cap_ext_parallelize"

desc "Raise a deliberate error"
task :raise_error do
  raise "Deliberate error"
end

desc "Conduct a transaction"
task :transact1 do
  transaction do
    on_rollback do
      puts "rollback 1"
    end
    transact2
  end
end

desc "Conduct a transaction"
task :transact2 do
  transaction do
    on_rollback do
      puts "rollback 2"
    end
    raise_error
  end
end

parallelize(1) do |session|
  session.run do
    transact1
  end
end

The output:


 ** Running 1 threads in chunks of 1
 ** Running batch number 1
 ** Running block in background thread
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1
*** Subthread failed: Deliberate error
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1
Capfile:5:in `block in load': Deliberate error (RuntimeError)

Notice the attempt to rollback twice, once in the background thread and once in the main thread. To compound matters, I had to modify two places to get the thing to rollback once in the main thread:

  1. Delete the entire Capistrano::Configuration::Execution module belonging to the cap-ext-parallelize project.
  2. Add in the if Thread.current == Thread.main conditional to produce the line
self.rollback_requests = [] if Thread.current == Thread.main

in lib/capistrano/configuration/execution.rb.

Making the above changes results in the correct output:


 ** Running 1 threads in chunks of 1
 ** Running batch number 1
 ** Running block in background thread
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
 ** transaction: start
  * executing `raise_error'
*** Subthread failed: Deliberate error
  * executing `transact1'
 ** transaction: start
  * executing `transact2'
  * executing `raise_error'
*** [transact2] rolling back
rollback 2
*** [transact1] rolling back
rollback 1

Since both Capistrano itself and cap-ext-parallelize extension need some fixing, I would recommend reverting the code that supports "main thread rolls back" semantics and defer to @roidrage for patching the extension. Again, commit ea155dd doesn't affect Capistrano's behavior sans extension; all of those constructs work in support of the extension.

@roidrage

This comment has been minimized.

Show comment Hide comment
@roidrage

roidrage Jul 5, 2012

Contributor

It must be said that cap-ext-parallelize hasn't been updated in a while, maybe not even for the changes in Capistrano itself, so I could take of that part easily. And yeah, the fixes with thread locals were done to at least allow a somewhat parallel execution of tasks with the extension. The purpose was to allow running several commands in parallel because that's something Capistrano couldn't/can't do unfortunately.

Contributor

roidrage commented Jul 5, 2012

It must be said that cap-ext-parallelize hasn't been updated in a while, maybe not even for the changes in Capistrano itself, so I could take of that part easily. And yeah, the fixes with thread locals were done to at least allow a somewhat parallel execution of tasks with the extension. The purpose was to allow running several commands in parallel because that's something Capistrano couldn't/can't do unfortunately.

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 5, 2012

Contributor

Are the "main thread rolls back" semantics necessary in the first place? If not, we could just apply my patch independent of your changes.

Contributor

carsomyr commented Jul 5, 2012

Are the "main thread rolls back" semantics necessary in the first place? If not, we could just apply my patch independent of your changes.

@leehambley

This comment has been minimized.

Show comment Hide comment
@leehambley

leehambley Jul 6, 2012

Owner

Thanks for digging into this everyone, I appreciate it a lot.

@carsomyr Actually I am talking about that other pull request, because of my current status https://groups.google.com/forum/?fromgroups#!topic/capistrano/N8BFTEwGPYo and thus Capistrano's "no changes" current status until someone steps up to maintain, or my private life workload stabilises. (Sorry!)

Owner

leehambley commented Jul 6, 2012

Thanks for digging into this everyone, I appreciate it a lot.

@carsomyr Actually I am talking about that other pull request, because of my current status https://groups.google.com/forum/?fromgroups#!topic/capistrano/N8BFTEwGPYo and thus Capistrano's "no changes" current status until someone steps up to maintain, or my private life workload stabilises. (Sorry!)

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 6, 2012

Contributor

@leehambley, thanks for being so busy and remaining responsive. Perhaps I'll step up as the maintainer in the future, but for now I'll have to settle with learning about the different components and getting my feature working. That will go a long way towards making me adept at DSL hacking. In the meantime, I will keep you updated on the new facets feature, regardless of whether it'll see inclusion soon.

I still think that the current issue at hand can be closed out in a satisfactory way, though: The redundant constructs can be removed for the time being, and once @roidrage fixes his cap-ext-parallelize plugin, they can be reintroduced.

Contributor

carsomyr commented Jul 6, 2012

@leehambley, thanks for being so busy and remaining responsive. Perhaps I'll step up as the maintainer in the future, but for now I'll have to settle with learning about the different components and getting my feature working. That will go a long way towards making me adept at DSL hacking. In the meantime, I will keep you updated on the new facets feature, regardless of whether it'll see inclusion soon.

I still think that the current issue at hand can be closed out in a satisfactory way, though: The redundant constructs can be removed for the time being, and once @roidrage fixes his cap-ext-parallelize plugin, they can be reintroduced.

@leehambley

This comment has been minimized.

Show comment Hide comment
@leehambley

leehambley Jul 6, 2012

Owner

Thanks @carsomyr - I'll certainly try and find some time, I'm thinking about using my vacation days to finalise as many open issues as I can, and then leave it as it is until the year is out.

If I'm not mistaken @roidrage is working with Travis-CI (great job dude!) and is probably also short on time - but I acknowledge the resolution, and will leave the ticket open pending a new temporary maintainer, or more free time!

Thanks again everyone.

Owner

leehambley commented Jul 6, 2012

Thanks @carsomyr - I'll certainly try and find some time, I'm thinking about using my vacation days to finalise as many open issues as I can, and then leave it as it is until the year is out.

If I'm not mistaken @roidrage is working with Travis-CI (great job dude!) and is probably also short on time - but I acknowledge the resolution, and will leave the ticket open pending a new temporary maintainer, or more free time!

Thanks again everyone.

@carsomyr

This comment has been minimized.

Show comment Hide comment
@carsomyr

carsomyr Jul 25, 2012

Contributor

@leehambley I am going to merge the pull request I submitted earlier. @roidrage When you do get around to patching cap-ext-parallelize, give me a holler and I'll work with you to integrate thread-local variables back into the Capistrano plumbing.

Contributor

carsomyr commented Jul 25, 2012

@leehambley I am going to merge the pull request I submitted earlier. @roidrage When you do get around to patching cap-ext-parallelize, give me a holler and I'll work with you to integrate thread-local variables back into the Capistrano plumbing.

@carsomyr carsomyr closed this Jul 25, 2012

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