Skip to content
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

[JENKINS-39134] CreationException thrown for block-scoped executions injecting step with no-arg ctor #14

Merged
merged 5 commits into from Nov 30, 2016

Conversation

Projects
None yet
3 participants
@jglick
Copy link
Member

commented Nov 29, 2016

@jglick jglick changed the title [JENKINS-39134] Reproduced bug in minimal scenario [JENKINS-39134] CreationException thrown for block-scoped executions injecting step with no-arg ctor Nov 29, 2016

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

Without really understanding Guice black magic I noted that overriding onResume to call

AbstractStepImpl.prepareInjector(getContext(), new MyStep()).injectMembers(this);

works, yet

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/ContextParameterModule.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/ContextParameterModule.java
index 595a183..9045d54 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/steps/ContextParameterModule.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/ContextParameterModule.java
@@ -10,9 +10,11 @@ import com.google.inject.spi.TypeListener;
 
 import javax.annotation.Nullable;
 import java.io.IOException;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -36,6 +38,32 @@ class ContextParameterModule extends AbstractModule {
             for (Class c=step.getClass(); c!=Step.class; c=c.getSuperclass()) {
                 bind(c).toInstance(step);
             }
+        } else { // JENKINS-39134 workaround: do not let Guice pick up a binding from the parent injector
+            bindListener(Matchers.any(), new TypeListener() {
+                @Override public <I> void hear(TypeLiteral<I> type, TypeEncounter<I> encounter) {
+                    for (final Field f : type.getRawType().getDeclaredFields()) {
+                        final Class<?> stepType = f.getType();
+                        if (Step.class.isAssignableFrom(stepType)) {
+                            encounter.register(new MembersInjector<I>() {
+                                @Override public void injectMembers(I instance) {
+                                    if (Modifier.isTransient(f.getModifiers())) {
+                                        try {
+                                            Constructor<?> stepCtor = stepType.getConstructor();
+                                            f.setAccessible(true);
+                                            Object step = stepCtor.newInstance();
+                                            f.set(instance, step);
+                                        } catch (NoSuchMethodException x) {
+                                            // fine, not a no-arg ctor, would not pose a problem anyway
+                                        } catch (Exception x) {
+                                            throw new ProvisionException("failed to work around JENKINS-39134 on " + f, x);
+                                        }
+                                    }
+                                }
+                            });
+                        }
+                    }
+                }
+            });
         }
 
         bindListener(Matchers.any(), new TypeListener() {

does not.

@reviewbybees

This comment has been minimized.

Copy link

commented Nov 29, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick added some commits Nov 29, 2016

@abayer

abayer approved these changes Nov 30, 2016

Copy link
Member

left a comment

🐝, though as mentioned, it might make sense to deprecate the Guice-dependent classes since we're advising against using them.

f.setAccessible(true);
f.set(instance, stepCtor.newInstance());
*/
LOGGER.log(Level.WARNING, "JENKINS-39134: {0} may not be marked @Inject; do not use AbstractStep'{',Execution,Descriptor'}'Impl", f);

This comment has been minimized.

Copy link
@abayer

abayer Nov 30, 2016

Member

fwiw, I'd say deprecate those classes while we're here.

@jglick

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2016

it might make sense to deprecate the Guice-dependent classes since we're advising against using them

Yes, that is my thinking too. But I will leave that to a separate PR which also provides Guiceless versions of AbstractSynchronousStepExecution and AbstractSynchronousNonBlockingStepExecution, along with some downstream PRs.

@jglick jglick merged commit 244bc12 into jenkinsci:master Nov 30, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:CreationException-JENKINS-39134 branch Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.