Fix out of order writes with async reactive calls #10579
Conversation
In a few places, I used the pattern where reactive calls would be dispatched to the event loop if they weren't already on the loop. However, if there is a reactive call outside the event loop, and then immediately a reactive call *on* the event loop, the calls can run in the wrong order. The second call on the event loop was not delayed with an execute() call, so it could run before the first call runs. This fix introduces a central EventLoopSerializer that takes care of this problem. In the above problem scenario, it takes care to also delay the second call even though it was submitted on the event loop.
http-netty/src/main/java/io/micronaut/http/netty/EventLoopSerializer.java
Outdated
Show resolved
Hide resolved
http-netty/src/main/java/io/micronaut/http/netty/EventLoopSerializer.java
Outdated
Show resolved
Hide resolved
| delayTask.run(); | ||
| } finally { | ||
| if (runGeneration != generation + 1 || submitGeneration != generation + 1) { | ||
| throw new AssertionError("Nested call?"); |
There was a problem hiding this comment.
throw IllegalStateException with a better message
There was a problem hiding this comment.
this is only with STRICT_CHECKING which is off by default and i only use it for debugging
http-netty/src/main/java/io/micronaut/http/netty/EventLoopSerializer.java
Outdated
Show resolved
Hide resolved
| public boolean executeNow(Runnable delayTask) { | ||
| // pick a generation ID for this task. | ||
| int generation = submitGeneration++; | ||
| if (loop.inEventLoop()) { |
There was a problem hiding this comment.
Would it be better to implement this with a lock? Immediate execution (or execution in general) is acquiring the lock, if lock is taken the task should be scheduled. IMHO it would be much easier to understand.
There was a problem hiding this comment.
these ops often cant run concurrently with other event loop operations which definitely cannot use a lock
| * @param delayTask The task to run if it can't be run immediately | ||
| * @return {@code true} if the caller should instead run the task immediately | ||
| */ | ||
| public boolean executeNow(Runnable delayTask) { |
There was a problem hiding this comment.
I'm not a fan of this signature. Can we execute the runnable?
There was a problem hiding this comment.
there is a happy path that is only on the event loop where i want to avoid an interface call into the lambda. the goal here is to reduce cost as much as possible for the case where everything is inEventLoop
There was a problem hiding this comment.
It looks to me like you can implement this as:
if (reentrantLock.isLocked()) {
// Schedule
}
try {
reentrantLock.lock();
// Execute
} finally {
reentrantLock.unlock();
}
Or with tryLock
There was a problem hiding this comment.
no it really has to run on the event loop so that it does not conflict with other calls that happen on the loop
There was a problem hiding this comment.
also note that this pattern would not actually solve the serialization. if one task fails to lock, schedules, then a second task succeeds the lock and runs immediately before the first scheduled task runs, we still have incorrect ordering.
There was a problem hiding this comment.
Need to address the checkstyle issues https://ge.micronaut.io/s/wut53koeap4xq/console-log/task/:http-server-netty:checkstyleMain?anchor=7&page=1
Also Japicmp is complaining about metadata binary compatibility if you run ./gradlew :http-netty:japiCmp
http-client/src/main/java/io/micronaut/http/client/netty/ReactiveClientWriter.java
Outdated
Show resolved
Hide resolved
|
|
@timyates i dont see the japicmp issue |
japicmp seems to be being tempremental... 🤔 |
In a few places, I used the pattern where reactive calls would be dispatched to the event loop if they weren't already on the loop. However, if there is a reactive call outside the event loop, and then immediately a reactive call on the event loop, the calls can run in the wrong order. The second call on the event loop was not delayed with an execute() call, so it could run before the first call runs.
This fix introduces a central EventLoopSerializer that takes care of this problem. In the above problem scenario, it takes care to also delay the second call even though it was submitted on the event loop.