Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Incorrect Calculation of Cumulative Demand in Duplicator #6150

Merged
merged 2 commits into from
Mar 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ void doRequestDemand(long cumulativeDemand) {
}

final long delta = cumulativeDemand - requestedDemand;
requestedDemand += delta;
requestedDemand = cumulativeDemand;
upstreamSubscription.request(delta);
}

Expand Down Expand Up @@ -658,18 +658,12 @@ public void request(long n) {
return;
}

accumulateDemand(n);
cumulativeDemand = LongMath.saturatedAdd(cumulativeDemand, n);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question) Shouldn’t we take multi-threaded race conditions into account? I might not have a complete understanding of DownstreamSubscription’s threading model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it okay because cumulativeDemand is calculated by the thread who calls the subscription.request()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. If subscription.request() should be sequentically called, is it safe to remove the following lines? It seems like requestedDemand is always less than cumulativeDemand in doRequestDemand(). If I'm wrong, may I know when it could happen?

if (cumulativeDemand <= requestedDemand) {
return;
}

Copy link
Contributor Author

@minwoox minwoox Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subscription.request() should be sequentically called

This is for one child stream message.
processor.requestDemand(cumulativeDemand); can be called from multiple child stream messages. Let's say there are two child stream messages and the first one request 10 and the second requests 1, then the condition will be true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Everything makes sense now.

processor.requestDemand(cumulativeDemand);

for (;;) {
final long oldDemand = demand;
final long newDemand;
if (oldDemand >= Long.MAX_VALUE - n) {
newDemand = Long.MAX_VALUE;
} else {
newDemand = oldDemand + n;
}

final long newDemand = LongMath.saturatedAdd(oldDemand, n);
if (demandUpdater.compareAndSet(this, oldDemand, newDemand)) {
if (oldDemand == 0) {
signal();
Expand All @@ -679,14 +673,6 @@ public void request(long n) {
}
}

private void accumulateDemand(long n) {
if (n == Long.MAX_VALUE || Long.MAX_VALUE - n >= cumulativeDemand) {
cumulativeDemand = Long.MAX_VALUE;
} else {
cumulativeDemand += n;
}
}

void signal() {
if (downstreamExecutor.inEventLoop()) {
doSignal();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.reactivestreams.Subscriber;
import org.reactivestreams.Subscription;

import com.google.common.math.LongMath;

import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.annotation.UnstableApi;
import com.linecorp.armeria.common.util.CompletionActions;
Expand Down Expand Up @@ -273,7 +275,7 @@ private void doRequest(long n) {
if (upstreamSubscription != null) {
upstreamSubscription.request(n);
} else {
pendingDemand += n;
pendingDemand = LongMath.saturatedAdd(pendingDemand, n);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ private void onNext0(HttpData httpData) {
if (!usePooledObject) {
httpData = PooledObjects.copyAndClose(httpData);
}
final Subscriber<? super HttpData> downstream = this.downstream;
assert downstream != null;
downstream.onNext(httpData);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* Copyright 2025 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package com.linecorp.armeria.common.stream;

import static org.assertj.core.api.Assertions.assertThat;
import static org.awaitility.Awaitility.await;

import java.util.concurrent.atomic.AtomicLong;

import org.junit.jupiter.api.Test;
import org.reactivestreams.Subscriber;
import org.reactivestreams.Subscription;

final class DuplicatorCumulativeDemandTest {

@Test
void cumulativeDemand() throws InterruptedException {
final AtomicLong demand = new AtomicLong();
final PublisherBasedStreamMessage<Integer> streamMessage = new PublisherBasedStreamMessage<>(
s -> s.onSubscribe(new Subscription() {
@Override
public void request(long n) {
demand.addAndGet(n);
}

@Override
public void cancel() {}
}));
streamMessage.toDuplicator().duplicate().subscribe(new Subscriber<Integer>() {
@Override
public void onSubscribe(Subscription s) {
s.request(1);
}

@Override
public void onNext(Integer integer) {}

@Override
public void onError(Throwable t) {}

@Override
public void onComplete() {}
});
await().untilAsserted(() -> assertThat(demand.get()).isEqualTo(1));
Thread.sleep(100);
// The demand is still 1.
assertThat(demand.get()).isEqualTo(1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* under the License.
*/

package com.linecorp.armeria;
package com.linecorp.armeria.server;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -25,7 +25,6 @@

import com.linecorp.armeria.common.ShuttingDownException;
import com.linecorp.armeria.internal.testing.AnticipatedException;
import com.linecorp.armeria.server.GracefulShutdown;

class GracefulShutdownBuilderTest {

Expand Down
Loading