ExecDumpClient should report error when no execution data is retrieved #538

marchof · 2017-05-22T21:22:20Z

Ant and Maven dump commands use the ExecDumpClient class from the org.jacoco.core module. This utility does not evaluate the response status after sending a dump request. If the remote closes the connection without sending a response this is silently ignored.

Expected behavior is that a IOException is thrown when a dump request is not answered.

Godin · 2017-05-22T21:19:04Z

@marchof one case is obvious - when remote is something that always closes connection; wondering under which conditions real JaCoCo agent can do this?

marchof · 2017-05-22T21:25:44Z

@Godin No idea how the actual JaCoCo agent would behave like this. This PR is more about making the client bullet-proof especially about missconfiguration.

Godin · 2017-05-23T16:26:47Z

org.jacoco.doc/docroot/doc/changes.html

@@ -41,6 +41,9 @@
      (GitHub <a href="https://github.com/jacoco/jacoco/issues/529">#529</a>).</li>
  <li>Maven aggregated reports will now also include modules of runtime dependencies
      (GitHub <a href="https://github.com/jacoco/jacoco/issues/498">#498</a>).</li>
+  <li><code>dump</code> commands now report errors when remote connections are
+      closed unexpectedly


@marchof I'd like to reword this to be more precise as this is about one particular situation described in ticket and not about all situations when connection is closed. Something like:

<code>dump</code> commands now report error when server unexpectedly closes connection without sending response

Also you marked ticket as type: bug, but then change should be in section "Fixed Bugs" instead of "New Features", isn't it?

@Godin Fixed in c9d9887 ✅

Godin · 2017-05-23T16:29:29Z

org.jacoco.core.test/src/org/jacoco/core/tools/ExecDumpClientTest.java

@@ -126,6 +126,12 @@ public void testReset() throws IOException {
 		assertTrue(resetRequested);
 	}

+	@Test(expected = IOException.class)
+	public void testNopServer() throws IOException {


@marchof what about usage of a new naming convention, i.e. rename into should_throw_IOException_when_server_closes_connection_without_response ?

Also what about assertion for a message? And seems that there is race condition, because such assertion will fail from time to time, which explains why in #525 (comment) I stated that throw was not covered:

java.net.SocketException: Broken pipe at java.net.SocketOutputStream.socketWrite0(Native Method) at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:92) at java.net.SocketOutputStream.write(SocketOutputStream.java:115) at java.io.DataOutputStream.writeChar(DataOutputStream.java:166) at org.jacoco.core.data.ExecutionDataWriter.writeHeader(ExecutionDataWriter.java:72) at org.jacoco.core.data.ExecutionDataWriter.<init>(ExecutionDataWriter.java:61) at org.jacoco.core.runtime.RemoteControlWriter.<init>(RemoteControlWriter.java:40) at org.jacoco.core.tools.ExecDumpClient.dump(ExecDumpClient.java:118) at org.jacoco.core.tools.ExecDumpClient.dump(ExecDumpClient.java:98)

@Godin Good catch! Needed to insert a Thread.sleep() to reproduce this, but I was able to reproduce it. Fixed in 42b4f9f ✅

I didn't change the method name originally, as the code change was a 1:1 patch from #525. Now as we needed to adjust the code I also change the method name as suggested. To avoid merge conflicts I will revert the corresponding change in #525, correct?

@marchof well, IMO no big difference whether to explicitly revert or resolve conflict during merge, but I see that you already did a revert

Godin · 2017-05-23T17:06:37Z

org.jacoco.core/src/org/jacoco/core/tools/ExecDumpClient.java

@@ -123,7 +123,10 @@ public ExecFileLoader dump(final InetAddress address, final int port)
 					.setExecutionDataVisitor(loader.getExecutionDataStore());

 			remoteWriter.visitDumpCommand(dump, reset);
-			remoteReader.read();
+
+			if (!remoteReader.read()) {


@marchof we have similar code in org.jacoco.examples/src/org/jacoco/examples/ExecutionDataClient.java. Later we can update it to use ExecDumpClient, but IMO anyway will be better to apply same modification in it with this PR.

Fixed in acfb4b0 ✅

@Godin

Review comment by @Godin

* Assert exception message * Avoid raise condition * Use new naming scheme

@Godin

Review comment by @Godin

Godin · 2017-05-24T22:17:00Z

@marchof 👍

denisa · 2017-06-08T00:26:32Z

Thanks for having that fixed, I just spend an hour tracking that down on 0.7.9.

My use case:
I have a docker image that contains wildfly and the jacobo agent jar file.
The docker image always exposes port 6300, but I decide upon launching the image if I want to enable the jacoco agent or not.

When I connect to a running docker instance that does not have jacoco enabled, ExecDumpClient.dump() returns an (empty) ExecFileLoader instead of failing as desired.

Godin · 2017-06-08T13:54:22Z

@denisa could you please clarify just to be sure - do you mean that after this change in your use case you're getting IOException as expected?

denisa · 2017-06-08T14:29:44Z

Yes, this bug is fixed: 0.7.10-SNAPSHOT throws an IOException as designed when 0.7.9 simply returned an object.

Godin · 2017-06-08T18:04:42Z

@denisa cool! thank you for information 👍

marchof added component: core type: bug 🐛 Something isn't working labels May 22, 2017

marchof self-assigned this May 22, 2017

marchof mentioned this pull request May 22, 2017

Simple Command Line Interface for JaCoCo #525

Merged

5 tasks

Report error when remote connections are closed unexpectedly (#538)

0fa5639

marchof requested a review from Godin May 22, 2017 21:22

Godin requested changes May 23, 2017

View reviewed changes

marchof added 3 commits May 24, 2017 14:01

Better description, categorize as bug.

c9d9887

Review comment by @Godin

Improve test case according to @Godin's review comments

42b4f9f

* Assert exception message * Avoid raise condition * Use new naming scheme

Same check as in ExecDumpClient.

acfb4b0

Review comment by @Godin

marchof added a commit that referenced this pull request May 24, 2017

Revert changes now addressed in #538.

4115a6e

Godin approved these changes May 24, 2017

View reviewed changes

Godin merged commit 10f3ff0 into master May 24, 2017

Godin deleted the issue-538 branch May 24, 2017 22:15

Godin added this to the 0.8.0 milestone May 24, 2017

jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ExecDumpClient should report error when no execution data is retrieved #538

ExecDumpClient should report error when no execution data is retrieved #538

marchof commented May 22, 2017

Godin commented May 22, 2017

marchof commented May 22, 2017

Godin May 23, 2017

marchof May 24, 2017

Godin May 23, 2017

marchof May 24, 2017

Godin May 24, 2017

Godin May 23, 2017

marchof May 24, 2017

Godin commented May 24, 2017

denisa commented Jun 8, 2017

Godin commented Jun 8, 2017

denisa commented Jun 8, 2017

Godin commented Jun 8, 2017

ExecDumpClient should report error when no execution data is retrieved #538

ExecDumpClient should report error when no execution data is retrieved #538

Conversation

marchof commented May 22, 2017

Godin commented May 22, 2017

marchof commented May 22, 2017

Godin May 23, 2017

Choose a reason for hiding this comment

marchof May 24, 2017

Choose a reason for hiding this comment

Godin May 23, 2017

Choose a reason for hiding this comment

marchof May 24, 2017

Choose a reason for hiding this comment

Godin May 24, 2017

Choose a reason for hiding this comment

Godin May 23, 2017

Choose a reason for hiding this comment

marchof May 24, 2017

Choose a reason for hiding this comment

Godin commented May 24, 2017

denisa commented Jun 8, 2017

Godin commented Jun 8, 2017

denisa commented Jun 8, 2017

Godin commented Jun 8, 2017