Run closures synchronously unless and until we cannot #9

Closed
jglick opened this Issue Mar 13, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@jglick
Member

jglick commented Mar 13, 2015

Given an arbitrary method, perhaps in DefaultGroovyMethods but perhaps in some other (binary) library, which takes a (CPS-transformed) closure, the behavior is nonsensical:

[1, 2].whatever {println it}

will call whatever, and when that method calls Closure.call, the closure is run correctly…but once it returns, the whole call to whatever is suddenly terminated (with CpsCallableInvocation), and so the result is that the closure is only ever called once, and the overall return value is meaningless.

What we would prefer is that if the closure can run synchronously, we run it without CPS transformation and let the whatever method run to completion synchronously, so that basic calls like

def x = [1, 2].find {it %2 == 0}

will work. But if something inside the closure calls Continuable.suspend, then we need to notice that the call stack includes non-CPS methods, and we want to throw an error with a helpful message. Thus for example in a Workflow

[1, 2].each = {echo "found ${it}"}

would work (since EchoStep is synchronous and so tells DSL.invokeMethod to return null immediately), but

[1, 2].each = {sh "echo found ${it}"}

would throw an error (since DurableTaskStep is asynchronous and so tells DSL.invokeMethod to call Continuable.suspend).

@gregsymons

This comment has been minimized.

Show comment
Hide comment
@gregsymons

gregsymons Jul 30, 2015

The example above:

[1, 2].each = { sh "echo found ${it}" }

seems like a reasonable idiom for performing the same build steps based on a collection of parameters. It would be useful to have a reasonable replacement. I don't really understand enough how the transformation code works but is it possible at transformation time to detect whether or not anything in the block will call Continuable.suspend, either through annotation or inspection, and transform closures that do, while leaving closures that don't alone? Or, would it be reasonable to add some extra syntax to help the transformation, something like

[1, 2].each = `cont` { sh "echo found ${it} }

where cont (an awful name, but I can't think of a better one at the moment) is a DSL method that explicitly transforms the passed in closure into a CpsClosure or whatever it needs to be to work right.

The example above:

[1, 2].each = { sh "echo found ${it}" }

seems like a reasonable idiom for performing the same build steps based on a collection of parameters. It would be useful to have a reasonable replacement. I don't really understand enough how the transformation code works but is it possible at transformation time to detect whether or not anything in the block will call Continuable.suspend, either through annotation or inspection, and transform closures that do, while leaving closures that don't alone? Or, would it be reasonable to add some extra syntax to help the transformation, something like

[1, 2].each = `cont` { sh "echo found ${it} }

where cont (an awful name, but I can't think of a better one at the moment) is a DSL method that explicitly transforms the passed in closure into a CpsClosure or whatever it needs to be to work right.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 12, 2015

Member

is it possible at transformation time to detect whether or not anything in the block will call Continuable.suspend

No.

Member

jglick commented Aug 12, 2015

is it possible at transformation time to detect whether or not anything in the block will call Continuable.suspend

No.

@jglick

This comment has been minimized.

Show comment
Hide comment
Member

jglick commented Jan 4, 2016

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Aug 8, 2017

Member

Given that most DefaultGroovyMethods taking Closure are now being transformed, this is less important; probably suffices to just report an immediate error, as in JENKINS-31314.

Member

jglick commented Aug 8, 2017

Given that most DefaultGroovyMethods taking Closure are now being transformed, this is less important; probably suffices to just report an immediate error, as in JENKINS-31314.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 13, 2018

Member

Closing in favor of #85.

Member

jglick commented Mar 13, 2018

Closing in favor of #85.

@jglick jglick closed this Mar 13, 2018

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