You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by jh...@apache.org on 2024/09/25 19:31:10 UTC
(james-project) 03/04: [devscout] actually consume the iterators in ManageableMailQueueContract
This is an automated email from the ASF dual-hosted git repository.
jhelou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit f2a56fc64c8dce852017a2b4c48caee595025ce5
Author: Jean Helou <je...@xn--gml-cma.com>
AuthorDate: Fri Sep 20 09:00:01 2024 +0200
[devscout] actually consume the iterators in ManageableMailQueueContract
The previous implementation using `Iterators.consumingIterator`
doesn't actually consume anything. It only wraps the iterator it receives in an interator that removes the items from the underlying iterator when next is called.
We implemented a very simple : consumeIterator method to actually consume and therefore trigger the potential concurrency issues the tests were trying to protect against.
Doing so raised a failing test in ActiveMQMailQueueBlobTest.
---
.../queue/activemq/ActiveMQMailQueueBlobTest.java | 12 ++++++++++
.../queue/api/ManageableMailQueueContract.java | 27 +++++++++++++++-------
2 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java b/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
index c7abe08db0..cc41ceaef9 100644
--- a/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
+++ b/server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java
@@ -31,6 +31,7 @@ import org.apache.activemq.ActiveMQPrefetchPolicy;
import org.apache.activemq.broker.BrokerService;
import org.apache.commons.io.FileUtils;
import org.apache.james.filesystem.api.FileSystem;
+import org.apache.james.junit.categories.Unstable;
import org.apache.james.metrics.api.GaugeRegistry;
import org.apache.james.metrics.api.MetricFactory;
import org.apache.james.queue.api.DelayedManageableMailQueueContract;
@@ -157,6 +158,17 @@ public class ActiveMQMailQueueBlobTest implements DelayedManageableMailQueueCont
}
+ @Tag(Unstable.TAG)
+ @Test
+ @Override
+ public void browseShouldNotFailWhenConcurrentClearWhenIterating() throws Exception {
+ // This test used to pass because the assertion did not actually check anything
+ // it used Iterators.consumingIterator which only wraps the underlying iterator without
+ // actually consuming it. When replaced with an actual consumption this implementation started
+ // failing.
+ DelayedManageableMailQueueContract.super.browseShouldNotFailWhenConcurrentClearWhenIterating();
+ }
+
@Test
void computeNextDeliveryTimestampShouldReturnLongMaxWhenOverflow() {
long deliveryTimestamp = mailQueue.computeNextDeliveryTimestamp(ChronoUnit.FOREVER.getDuration());
diff --git a/server/queue/queue-api/src/test/java/org/apache/james/queue/api/ManageableMailQueueContract.java b/server/queue/queue-api/src/test/java/org/apache/james/queue/api/ManageableMailQueueContract.java
index 42cb4803e0..cae74f4d77 100644
--- a/server/queue/queue-api/src/test/java/org/apache/james/queue/api/ManageableMailQueueContract.java
+++ b/server/queue/queue-api/src/test/java/org/apache/james/queue/api/ManageableMailQueueContract.java
@@ -31,6 +31,7 @@ import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import java.time.Duration;
+import java.util.Iterator;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@@ -41,10 +42,12 @@ import jakarta.mail.internet.MimeMessage;
import org.apache.james.core.MailAddress;
import org.apache.james.core.builder.MimeMessageBuilder;
+import org.apache.james.junit.categories.Unstable;
import org.apache.mailet.Attribute;
import org.apache.mailet.Mail;
import org.apache.mailet.base.MailAddressFixture;
import org.awaitility.Awaitility;
+import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
@@ -53,7 +56,6 @@ import org.junit.jupiter.params.provider.MethodSource;
import com.github.fge.lambdas.Throwing;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Iterators;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
@@ -217,7 +219,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
Flux.from(getManageableMailQueue().deQueue());
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -266,7 +268,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
Flux.from(getManageableMailQueue().deQueue());
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -307,7 +309,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
.name("name4")
.build());
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -352,7 +354,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
.name("name2")
.build());
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -392,7 +394,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
getManageableMailQueue().clear();
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -432,7 +434,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
getManageableMailQueue().flush();
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -474,7 +476,7 @@ public interface ManageableMailQueueContract extends MailQueueContract {
awaitRemove();
- assertThatCode(() -> Iterators.consumingIterator(items)).doesNotThrowAnyException();
+ assertThatCode(() -> consumeIterator(items)).doesNotThrowAnyException();
}
@Test
@@ -738,4 +740,13 @@ public interface ManageableMailQueueContract extends MailQueueContract {
.containsExactly("name2");
}
+ default <T> int consumeIterator(Iterator<T> iterator) {
+ var i = 0;
+ while (iterator.hasNext()) {
+ iterator.next();
+ i++;
+ }
+ return i;
+ }
+
}
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org