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

[Jaikiran Pai]

On Mon, 20 May 2024 14:29:22 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 one additional commit since the last revision:
> 
>   reduce the scope of the try/catch and fix potential short read() issue

Hello Daniel,

> You could possibly have used `InputStream::readNBytes` to simplify the code. Unless you're constrained to use JDK 7/8 APIs?

I did attempt to simplify that part to use `InputStream.readNBytes()` but like you note, jtreg is constrainted to Java 8 APIs.

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

PR Comment: https://git.openjdk.org/jtreg/pull/195#issuecomment-2121542479