RFR: 7903730: Enhance the agentvm to be tolerant to unexpected connection on the port the Agent listens on for handshake with the AgentServer [v2]

[Ludvig Janiuk]

On Mon, 20 May 2024 13:52:03 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to implement the enhancement noted in https://bugs.openjdk.org/browse/CODETOOLS-7903730?
>> 
>> As noted in that enhancement request, in the agentvm mode, we establish a socket connection between the `Agent` and the `AgentServer`. In its current form, it can so happen that the `Agent` may end up expecting a connection from some unexpected process. That then leads to the agent vm launch failing and the test too failing.
>> 
>> The proposed change introduces a handshake where the `AgentServer` sends the `jtreg` "magic" bytes immediately after it connects to the `Agent`. The `Agent` verifies these bytes on the accepted connection and if it either doesn't receive these bytes (for the specified read timeout) or if it receives some other data than these handshake bytes, then it closes this accepted connection and then goes on to accept another connection and repeats the handshake verification. In the current proposed change, it reattempts only 2 times and I haven't seen a need to either configure this value or have a higher value for this. i.e. if we can't establish a connection and verify that the connection initiator is the `AgentServer` even after three attempts, then I think it's OK to fail the agent launch process.
>> 
>> tier1 through tier8 tests have been run with this change (a couple of times) over the past couple of months and I haven't noticed any regression, nor have I noticed the agentvm launch process to be failing due to unexpected connection initiator.
>
> Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - fix comments
>  - milliseconds instead of milli seconds

src/share/classes/com/sun/javatest/regtest/agent/AgentServer.java line 248:

> 246:         final OutputStream os = s.getOutputStream();
> 247:         os.write(JTREG_AGENT_HANDSHAKE_MAGIC);
> 248:         os.flush();

Is this the point where the agent would break the connection if the magic bytes are wrong? I'm trying to more clearly see at which point the jtreg "server" knows that the jtreg agent has accepted the handshake.

src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 193:

> 191:                 Socket s = null;
> 192:                 for (int i = 0; i < maxAcceptAttempts; i++) {
> 193:                     log("Waiting up to " + ACCEPT_TIMEOUT + " milli seconds for a" +

Typo: milliseconds.

src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 237:

> 235:     // Socket is returned. if the read bytes don't match the expected handshake bytes
> 236:     // or if the read times out after accept()ing an connection, then this method
> 237:     // closes the accepted connection and returns null.

Please capitalize the comments.

src/share/classes/com/sun/javatest/regtest/exec/Agent.java line 264:

> 262:                 s.close();
> 263:             } catch (IOException ignored) {
> 264:                 // ignore

Please don't swallow exceptions.

-------------

PR Review Comment: https://git.openjdk.org/jtreg/pull/195#discussion_r1606809481
PR Review Comment: https://git.openjdk.org/jtreg/pull/195#discussion_r1606802569
PR Review Comment: https://git.openjdk.org/jtreg/pull/195#discussion_r1606805812
PR Review Comment: https://git.openjdk.org/jtreg/pull/195#discussion_r1606810982