jacoco icon indicating copy to clipboard operation
jacoco copied to clipboard

Clarify semantics of RemoteControlWriter.read() in JavaDoc

Open ParadoxShmaradox opened this issue 4 months ago • 10 comments

Scenario

  • JaCoCo version: 0.8.13
  • Operating system: Linux
  • Tool integration Agent
  • Description of your use case: Hey, I'm building a service that jacoco agents in output=tcpclient can connect to. Within that service I have a functionality to send a dump request to an agent using
RemoteControlWriter remoteControlWriter =
        new RemoteControlWriter(handler.getSocket().getOutputStream());
remoteControlWriter.visitDumpCommand(true, true);

And the read side is also as follows:

new RemoteControlWriter(socket.getOutputStream());
reader = new RemoteControlReader(socket.getInputStream());
reader.setSessionInfoVisitor(this);
reader.setExecutionDataVisitor(this);

while (reader.read()) {}
socket.close();

Pretty standard stuff :)

However my functionality requires that the dump data be saved to s3, and I have no way to know when all data has been sent

Current Behaviour

On the read side the ISessionInfoVisitor, IExecutionDataVisitor are called as expected however there is currently no way to to know when all execution data has been sent.

Wanted Behaviour

A new command and visitor interface that the client sends when all execution data has been sent to the server.

That way we can know when all data has been sent (and received) by the server and we can act upon it.

Possible Workarounds

Not that I know of :(

ParadoxShmaradox avatar Aug 20 '25 17:08 ParadoxShmaradox

Once while (reader.read()) {} terminates all requested data has been processed. So you now that no more data arrives and you can save the data to S3. See JavaDoc:

https://www.jacoco.org/jacoco/trunk/doc/api/org/jacoco/core/data/ExecutionDataReader.html#read()

marchof avatar Aug 20 '25 18:08 marchof

@marchof while (reader.read()) {} terminate only if the agent exits if you send a dump command it does not exit, and should not because the agent is still connected to the server

ParadoxShmaradox avatar Aug 20 '25 18:08 ParadoxShmaradox

I'm pretty sure reader.read() returns false after the command has been processed, even if the agent keeps running and the socket is still alive. This how all our integrations are built:

https://github.com/jacoco/jacoco/blob/master/org.jacoco.core/src/org/jacoco/core/tools/ExecDumpClient.java#L113

But I agree that the JavaDoc in the Returns section talking about "end of stream" is missleading and wrong.

marchof avatar Aug 20 '25 18:08 marchof

@ParadoxShmaradox I have to correct myself. Everything in the API is correct. If you send a single command and only expect one response just call ExecutionDataReader.read() once. So instead of

while (reader.read()) {}

call

reader.read();

After this call all requested data has been processed.

marchof avatar Aug 20 '25 18:08 marchof

@marchof I have a thread the does while (reader.read()) {} so I can get the execution data if the agent exits. If you run reader.read(); after the dump command is sent that seems like a conflict as they both work on the same socket output stream

ParadoxShmaradox avatar Aug 20 '25 18:08 ParadoxShmaradox

Ok. So at what point in time do you want to write the data to S3? Maybe this is the logic you're looking for?

while (reader.read()) {
    storeToS3();
}

This is basically how our Eclipse integration works: Whenever a execution data dump comes over the socket connection, a new "session" is created and added. The data can either be requested by the user or just arrive at shutdown of the process.

https://github.com/eclipse-eclemma/eclemma/blob/master/org.eclipse.eclemma.core/src/org/eclipse/eclemma/internal/core/launching/AgentServer.java#L113

marchof avatar Aug 20 '25 18:08 marchof

@marchof You are correct that does exactly what I wanted! I thought the while (reader.read()) is the busy loop but the read method also has a busy loop.

I really appreciate you looking into this, sorry for wasting your time, have a great day

ParadoxShmaradox avatar Aug 20 '25 18:08 ParadoxShmaradox

@ParadoxShmaradox You're welcome! I'm happy I was able to clarify this.

marchof avatar Aug 20 '25 18:08 marchof

@marchof I have feeling that this is not the first such/similar question/confusion 😅 so seems that there is a room for improvement 😉 and we need to update javadoc @ParadoxShmaradox maybe you have an idea what to add or how to change it?

Godin avatar Aug 20 '25 19:08 Godin

@Godin

https://github.com/jacoco/jacoco/blob/8f24208e22d87e34345c49f19b23d813f45a1815/org.jacoco.core/src/org/jacoco/core/data/ExecutionDataReader.java#L80


Reads all data and reports it to the corresponding visitors. The stream
	 * is read until its end or a command confirmation has been sent.
	 *
	 * @return <code>true</code> if additional data can be expected after a
	 *         command has been executed. <code>false</code> if the end of the
	 *         stream has been reached.

Additional data could be anything there is no clear relation between a command and the data that is sent. One might think true mean that more is expected for a command. It would be more clear that data is parsed as a whole, command execution could reference the client here (i.e the client executes a command and sends data) it could be made clearer if the command execution here referenced the end of the data for it on the server side.

I went looking at https://github.com/jacoco/jacoco/blob/master/org.jacoco.examples/src/org/jacoco/examples/ExecutionDataServer.java for how to create a server, then some google search for how to dump.

I think an updated ExecutionDataServer should be best. I added my code for testing based on ExecutionDataServer.

It has a dump "api" and tracking of agents, it's not the best code in the world, but it incorporates multiple usages in one example.



import java.io.BufferedReader;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import lombok.extern.slf4j.Slf4j;
import org.jacoco.core.data.ExecutionData;
import org.jacoco.core.data.ExecutionDataWriter;
import org.jacoco.core.data.IExecutionDataVisitor;
import org.jacoco.core.data.ISessionInfoVisitor;
import org.jacoco.core.data.SessionInfo;
import org.jacoco.core.runtime.RemoteControlReader;
import org.jacoco.core.runtime.RemoteControlWriter;

/**
 * This example starts a socket server to collect coverage from agents that run
 * in output mode <code>tcpclient</code>. The collected data is dumped to a
 * local file.
 */
@Slf4j
public final class ExecutionDataServer {

    private static final String ADDRESS = "localhost";
    private static final int PORT = 6300;
    private static final int REST_PORT = 6069;
    private static final List<Handler> clientHandlers = Collections.synchronizedList(new ArrayList<>());

    private ExecutionDataServer() {}

    public static void main(final String[] args) throws IOException {

        Path coverage = Files.createTempDirectory("coverage");
        log.info("Coverage will be written to {}", coverage);

        ExecutorService tcpPool = Executors.newCachedThreadPool();

        // Start REST server in a separate thread
        new Thread(() -> {
                    try {
                        startRestServer();
                    } catch (IOException e) {
                        log.error(e.getMessage(), e);
                    }
                })
                .start();

        // Start TCP server for JaCoCo connections
        try (ServerSocket server = new ServerSocket(PORT, 0, InetAddress.getByName(ADDRESS))) {
            log.info("TCP server listening on port {}", PORT);

            //noinspection InfiniteLoopStatement
            while (true) {
                Socket accept = server.accept();
                tcpPool.submit(() -> {
                    try {
                        Path destFile = Path.of(
                                coverage.toString(),
                                accept.getInetAddress().getHostName() + "_"
                                        + UUID.randomUUID()
                                                .toString()
                                                .replaceAll("-", "")
                                                .replaceAll("[^a-zA-Z0-9._-]", "_")
                                        + ".exec");
                        Handler handler = new Handler(accept, destFile);
                        clientHandlers.add(handler);
                        handler.run();
                    } catch (IOException e) {
                        log.error(e.getMessage(), e);
                    }
                });
            }
        }
    }

    private static void startRestServer() throws IOException {
        try (ServerSocket restSocket = new ServerSocket(REST_PORT)) {
            log.info("REST server listening on port {}", REST_PORT);
            //noinspection InfiniteLoopStatement
            while (true) {
                Socket client = restSocket.accept();
                new Thread(() -> handleRestClient(client)).start();
            }
        }
    }

    private static void handleRestClient(Socket client) {
        try (client;
                BufferedReader in = new BufferedReader(new InputStreamReader(client.getInputStream()));
                OutputStream out = client.getOutputStream()) {

            // Read the first line of the request (e.g., "POST /dump HTTP/1.1")
            String requestLine = in.readLine();
            if (requestLine != null && requestLine.startsWith("POST /dump")) {
                log.info("Dumping execution for agents");

                for (Handler handler : clientHandlers) {
                    if (!handler.socket.isClosed()) {
                        log.info(
                                "Dumping for agent at {}",
                                handler.socket.getInetAddress().getHostAddress());
                        RemoteControlWriter remoteControlWriter =
                                new RemoteControlWriter(handler.socket.getOutputStream());
                        remoteControlWriter.visitDumpCommand(true, true);
                    }
                }

                String response = "HTTP/1.1 200 OK\r\nContent-Length: 30\r\n\r\nExecution data dumped!";
                out.write(response.getBytes());
            } else {
                String response = "HTTP/1.1 404 Not Found\r\nContent-Length: 9\r\n\r\nNot Found";
                out.write(response.getBytes());
            }
            out.flush();
        } catch (IOException e) {
            log.error(e.getMessage(), e);
        }
    }

    private static class Handler implements Runnable, ISessionInfoVisitor, IExecutionDataVisitor {

        private final Socket socket;

        private final RemoteControlReader reader;

        private final ExecutionDataWriter fileWriter;
        private final Path destFile;

        Handler(final Socket socket, final Path destFile) throws IOException {
            this.socket = socket;
            this.destFile = destFile;
            this.fileWriter = new ExecutionDataWriter(new FileOutputStream(destFile.toFile()));

            // Just send a valid header
            new RemoteControlWriter(socket.getOutputStream());

            reader = new RemoteControlReader(socket.getInputStream());
            reader.setSessionInfoVisitor(this);
            reader.setExecutionDataVisitor(this);
        }

        public void run() {
            try {
                log.info(
                        "Agent connection established for {}",
                        socket.getInetAddress().getHostName());

                while (reader.read()) {
                              log.info("Read all execution data");
                }
                socket.close();

                log.info("Writing to {}", destFile);
                synchronized (fileWriter) {
                    fileWriter.flush();
                }
            } catch (final IOException e) {
                log.error(e.getMessage(), e);
            }
        }

        public void visitSessionInfo(final SessionInfo info) {
            log.info("Retrieving execution Data for session: {}", info.getId());
            synchronized (fileWriter) {
                fileWriter.visitSessionInfo(info);
            }
        }

        public void visitClassExecution(final ExecutionData data) {
            synchronized (fileWriter) {
                fileWriter.visitClassExecution(data);
            }
        }
    }

ParadoxShmaradox avatar Aug 20 '25 19:08 ParadoxShmaradox