From fe40d4f703e0d487549ee722f49603fa9de1e593 Mon Sep 17 00:00:00 2001 From: Arnaud Date: Thu, 28 Dec 2017 12:45:56 +0100 Subject: [PATCH] [JENKINS-48725] fix listener exceptions not being handled properly --- .../org/jvnet/hudson/reactor/Reactor.java | 14 +- .../org/jvnet/hudson/reactor/SessionTest.java | 131 +++++++++++++++++- 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jvnet/hudson/reactor/Reactor.java b/src/main/java/org/jvnet/hudson/reactor/Reactor.java index 30ea9a1..8ccfefb 100644 --- a/src/main/java/org/jvnet/hudson/reactor/Reactor.java +++ b/src/main/java/org/jvnet/hudson/reactor/Reactor.java @@ -173,7 +173,11 @@ private synchronized Node milestone(final Milestone m) { if (n==null) { milestones.put(m,n=new Node(new Runnable() { public void run() { - listener.onAttained(m); + try { + listener.onAttained(m); + } catch(Throwable x) { + throw new TunnelException(x); + } } public String toString() { return "Milestone:"+m.toString(); @@ -205,13 +209,17 @@ public synchronized void addAll(Iterable _tasks) { for (final Task t : _tasks) { Node n = new Node(new Runnable() { public void run() { - listener.onTaskStarted(t); try { + listener.onTaskStarted(t); runTask(t); listener.onTaskCompleted(t); } catch (Throwable x) { boolean fatal = t.failureIsFatal(); - listener.onTaskFailed(t,x, fatal); + try { + listener.onTaskFailed(t, x, fatal); + } catch(Throwable x2) { + x = x2; + } if (fatal) throw new TunnelException(x); } diff --git a/src/test/java/org/jvnet/hudson/reactor/SessionTest.java b/src/test/java/org/jvnet/hudson/reactor/SessionTest.java index 6c9c603..10f662c 100644 --- a/src/test/java/org/jvnet/hudson/reactor/SessionTest.java +++ b/src/test/java/org/jvnet/hudson/reactor/SessionTest.java @@ -24,10 +24,12 @@ package org.jvnet.hudson.reactor; import junit.framework.TestCase; +import org.objectweb.carol.cmi.Naming; import org.objectweb.carol.cmi.test.TeeWriter; import org.jvnet.hudson.reactor.TaskGraphBuilder.Handle; import javax.naming.NamingException; +import java.io.IOException; import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.io.StringWriter; @@ -59,11 +61,15 @@ public void run(Reactor session, String id) throws Exception { } private String execute(Reactor s) throws Exception { + return execute(s, null); + } + + private String execute(Reactor s, ReactorListener l) throws Exception { StringWriter sw = new StringWriter(); System.out.println("----"); final PrintWriter w = new PrintWriter(new TeeWriter(sw,new OutputStreamWriter(System.out)),true); - s.execute(Executors.newCachedThreadPool(),new ReactorListener() { + ReactorListener listener = new ReactorListener() { //TODO: Does it really needs handlers to be synchronized? @Override @@ -85,7 +91,11 @@ public synchronized void onTaskFailed(Task t, Throwable err, boolean fatal) { public synchronized void onAttained(Milestone milestone) { w.println("Attained "+milestone); } - }); + }; + if (l != null) { + listener = new ReactorListener.Aggregator(Arrays.asList(listener, l)); + } + s.execute(Executors.newCachedThreadPool(), listener); return sw.toString(); } @@ -126,6 +136,89 @@ public void run(Reactor reactor, String id) throws Exception { } } + /** + * Is the exception in listener's onTaskStarted properly forwarded? + */ + public void testListenerOnTaskStartedFailure() throws Exception { + final Error[] e = new Error[1]; + try { + execute(buildSession("->t1->m", createNoOp()), new ReactorListenerBase() { + @Override + public void onTaskStarted(Task t) { + throw e[0]=new AssertionError("Listener error"); + } + } + + ); + fail(); + } catch (ReactorException x) { + assertSame(e[0],x.getCause()); + } + } + + /** + * Is the exception in listener's onTaskCompleted properly forwarded? + */ + public void testListenerOnTaskCompletedFailure() throws Exception { + final Error[] e = new Error[1]; + try { + execute(buildSession("->t1->m", createNoOp()), new ReactorListenerBase() { + @Override + public void onTaskCompleted(Task t) { + throw e[0]=new AssertionError("Listener error"); + } + } + + ); + fail(); + } catch (ReactorException x) { + assertSame(e[0],x.getCause()); + } + } + + /** + * Is the exception in listener's onTaskFailed properly forwarded? + */ + public void testListenerOnTaskFailedFailure() throws Exception { + final Error[] e = new Error[1]; + try { + execute(buildSession("->t1->m", new TestTask() { + public void run(Reactor reactor, String id) throws Exception { + throw new IOException("Yep"); + } + }), new ReactorListenerBase() { + @Override + public void onTaskFailed(Task t, Throwable err, boolean fatal) { + throw e[0]=new AssertionError("Listener error"); + } + } + + ); + fail(); + } catch (ReactorException x) { + assertSame(e[0],x.getCause()); + } + } + + /** + * Is the exception in listener's onAttained properly forwarded? + */ + public void testListenerOnAttainedFailure() throws Exception { + final Error[] e = new Error[1]; + try { + execute(buildSession("->t1->m", createNoOp()), new ReactorListenerBase() { + @Override + public void onAttained(Milestone milestone) { + throw e[0]=new AssertionError("Listener error"); + } + } + ); + fail(); + } catch (ReactorException x) { + assertSame(e[0],x.getCause()); + } + } + /** * Dynamically add a new task that can run immediately. */ @@ -237,6 +330,18 @@ public void run(Reactor reactor, String id) throws InterruptedException { }; } + /** + * Creates {@link TestTask} that does nothing. + */ + private TestTask createNoOp() { + return new TestTask() { + @Override + public void run(Reactor reactor, String id) throws Exception { + // do nothing + } + }; + } + interface TestTask { void run(Reactor reactor, String id) throws Exception; } @@ -330,4 +435,26 @@ private static String normalizeLineEnds(String s) { } return s.replace("\r\n", "\n").replace('\r', '\n'); } + + private abstract class ReactorListenerBase implements ReactorListener { + @Override + public void onTaskStarted(Task t) { + + } + + @Override + public void onTaskCompleted(Task t) { + + } + + @Override + public void onTaskFailed(Task t, Throwable err, boolean fatal) { + + } + + @Override + public void onAttained(Milestone milestone) { + + } + } }