Replace synchronized blocks in retrofit2 with ReentrantLock(Improve) #5911
Conversation
ChangguHan
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
| } | ||
|
|
||
| // Run atomically | ||
| if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) { |
There was a problem hiding this comment.
I tried to make the use of CAS instead of using Reentrant Lock.
So, when the compare failed, enqueue returns with failure callback.
| @@ -36,6 +36,11 @@ void write(byte[] source, int offset, int byteCount) { | |||
| if (byteCount == 0) { | |||
| return; | |||
| } | |||
| /* | |||
| * Once use okhttp4, the synchronized block could be replaced by Reentrant and Condition for Virtual Thread support. | |||
There was a problem hiding this comment.
Is there any plan to use okhttp4?
If that is not too far in the future, I hope to work in this PR.
There was a problem hiding this comment.
I think it's probably best to follow the okhttp version upstream is using which seems to be okhttp3 unless there is a significant benefit.
https://github.com/square/retrofit/blob/trunk/gradle/libs.versions.toml#L17
On a side note, does the okio side PR contain changes for APIs which Armeria is using? I'm asking because okhttp itself is an http engine and usage within Armeria is minimal.
There was a problem hiding this comment.
The okio side replace synchronized with using lock, condition, timeout.awaitSignal, condition.signalAll() at here.
Especially, the way to notify to monitor(here) is replaced with
timeout.awaitSignal.
But we cannot use timeout.awaitSignal unless we upgrade the version of okio.
| if (executionStateUpdater.compareAndSet(this, ExecutionState.IDLE, ExecutionState.RUNNING)) { | ||
| httpResponse = doCall(callFactory, request); | ||
| } else { | ||
| callback.onFailure(this, new IOException("Call state is not IDLE")); |
There was a problem hiding this comment.
Since entering this block is semantically equivalent to L190
| callback.onFailure(this, new IOException("Call state is not IDLE")); | |
| throw new IllegalStateException("executed already"); |
There was a problem hiding this comment.
@jrhee17
This block is for when httpResponse is null and executionState is not ExecutionState.IDLE.
When the call is cancelled, the state is ExecutionState.CANCELED
| @@ -36,6 +36,11 @@ void write(byte[] source, int offset, int byteCount) { | |||
| if (byteCount == 0) { | |||
| return; | |||
| } | |||
| /* | |||
| * Once use okhttp4, the synchronized block could be replaced by Reentrant and Condition for Virtual Thread support. | |||
There was a problem hiding this comment.
I think it's probably best to follow the okhttp version upstream is using which seems to be okhttp3 unless there is a significant benefit.
https://github.com/square/retrofit/blob/trunk/gradle/libs.versions.toml#L17
On a side note, does the okio side PR contain changes for APIs which Armeria is using? I'm asking because okhttp itself is an http engine and usage within Armeria is minimal.
Motivation:
Modifications:
synchronizedin ArmeriaCallFactory's createRequest() by using CASResult: