Replace synchonized blocks in retrofit2 with ReentrantLock

[daniel-itunu]

Motivation:

loom compatibility: synchronized should be removed to favour virtual threads friendliness. Resolves part of #4510

Modifications:

• Replaced all synchronized in retrofit2 module with ReentrantLock

[ikhoon]

It is likely that httpResponse can be atomically updated without a lock or synchronized.

if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) {
    httpResponse = doCall(callFactory, request);
} else {
    throw new IllegalStateException("executed already");
}

[daniel-itunu]

so this method should be reverted to previous state then? just realised the volatile on httpResponse @ikhoon

It is likely that httpResponse can be atomically updated without a lock or synchronized.

if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) {
    httpResponse = doCall(callFactory, request);
} else {
    throw new IllegalStateException("executed already");
}

[daniel-itunu]

changes reverted.

It is likely that httpResponse can be atomically updated without a lock or synchronized.

if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) {
    httpResponse = doCall(callFactory, request);
} else {
    throw new IllegalStateException("executed already");
}

[ikhoon]

so this method should be reverted to previous state then?

I meant to fix the code to use if with compareAndSet without ReentrantLock.

[daniel-itunu]

yes, no point calling lock/synchronized on a CAS operation. However this change
if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) { httpResponse = doCall(callFactory, request); } else { throw new IllegalStateException("executed already"); }
caused a test failure.

[jrhee17]

I see, it seems like we need to define a separate boolean variable to check if the response has been updated or not

[jrhee17]

Looks good to me once @ikhoon 's comment is addressed 👍
Can you take a look at CI failures?

[jrhee17]

It seems like okio enforces users to use wait/notifyAll so we can't simply replace synchronized. (causing test failure)

What do you think of reverting changes in this file?
square/okio#1169

[daniel-itunu]

got it. reverted changes in this file @jrhee17

It seems like okio enforces users to use wait/notifyAll so we can't simply replace synchronized. (causing test failure)

What do you think of reverting changes in this file? square/okio#1169

[daniel-itunu]

@ikhoon @jrhee17 I am facing the issue highlighted here Task :examples:static-files:test FAILED locally. any hint/idea?

[jrhee17]

Do you have a stacktrace of the failure?

[daniel-itunu]

[ikhoon]

• DefaultDnsCacheTest seems like a flaky test so it could be ignored.
• For PropertiesEndpointGroupTest, it is not related to your change. Could you try to retest after running gradle clean task?

[daniel-itunu]

Thanks @ikhoon

• DefaultDnsCacheTest, apparently is flaky, not showing up anymore.

I have not been able to successfully build the master branch locally as these two failures below kept being persistent even after gradle clean task

• PropertiesEndpointGroupTest. pathWithDefaultPort
• PropertiesEndpointGroupTest. pathWithoutDefaultPort

[jrhee17]

I see, so what shadedTest does is basically

1. Shade dependencies into a jar
2. Unzip the jar into "${project.buildDir}/classes/java/shaded-test"
3. Run tests for classes in the above directory

Can you actually try to navigate to "${project.buildDir}/classes/java/shaded-test" and compare the file location with the one printed in the error logs?

My guess is the percent encoded url is causing problems, but I'd like to make sure first. If this is indeed the issue, I think it's better to handle this in a separate PR.

After handling the following comment, I'd like to get this PR merged.
#4610 (comment)

[jrhee17]

update) confirmed locally that the following patch fixes the issue.

-        final Path resourcePath = new File(resourceUrl.getFile()).toPath();
+        final Path resourcePath = new File(resourceUrl.toURI().getPath()).toPath();

[daniel-itunu]

The patch works! PR here #4617

[trustin]

This changeset is no-op. Should we close it?

[jrhee17]

This changeset is no-op. Should we close it?

Let's do so 👍