You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by rc...@apache.org on 2020/11/11 04:42:59 UTC
[james-project] 07/07: JAMES-3359 Add tests to demystify
Mailbox/set update parentId data races
This is an automated email from the ASF dual-hosted git repository.
rcordier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 74b768235e4bcb1ef21e13d09f7acf88ca50bb2e
Author: Benoit Tellier <bt...@linagora.com>
AuthorDate: Tue Nov 3 18:03:14 2020 +0700
JAMES-3359 Add tests to demystify Mailbox/set update parentId data races
---
.../contract/MailboxSetMethodContract.scala | 157 ++++++++++++++++++++-
.../apache/james/jmap/mail/MailboxFactory.scala | 10 +-
2 files changed, 164 insertions(+), 3 deletions(-)
diff --git a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
index 3385e0d..d13058c 100644
--- a/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
+++ b/server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/MailboxSetMethodContract.scala
@@ -20,6 +20,7 @@
package org.apache.james.jmap.rfc8621.contract
import java.nio.charset.StandardCharsets
+import java.time.Duration
import io.netty.handler.codec.http.HttpHeaderNames.ACCEPT
import io.restassured.RestAssured._
@@ -38,11 +39,12 @@ import org.apache.james.mailbox.model.MailboxACL.{EntryKey, Right}
import org.apache.james.mailbox.model.{MailboxACL, MailboxId, MailboxPath}
import org.apache.james.mime4j.dom.Message
import org.apache.james.modules.{ACLProbeImpl, MailboxProbeImpl}
+import org.apache.james.util.concurrency.ConcurrentTestRunner
import org.apache.james.utils.DataProbeImpl
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.{Assertions, SoftAssertions}
import org.hamcrest.Matchers.{equalTo, hasSize}
-import org.junit.jupiter.api.{BeforeEach, Disabled, Tag, Test}
+import org.junit.jupiter.api.{BeforeEach, Disabled, RepeatedTest, Tag, Test}
trait MailboxSetMethodContract {
@@ -5941,6 +5943,159 @@ trait MailboxSetMethodContract {
|}""".stripMargin)
}
+ @RepeatedTest(100)
+ def concurrencyChecksUponParentIdUpdate(server: GuiceJamesServer): Unit = {
+ val mailboxId1: MailboxId = server.getProbe(classOf[MailboxProbeImpl])
+ .createMailbox(MailboxPath.forUser(BOB, "mailbox1"))
+ val mailboxId2: MailboxId = server.getProbe(classOf[MailboxProbeImpl])
+ .createMailbox(MailboxPath.forUser(BOB, "mailbox2"))
+ val request1 =
+ s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | [
+ | "Mailbox/set",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "update": {
+ | "${mailboxId1.serialize}": {
+ | "parentId": "${mailboxId2.serialize}"
+ | }
+ | }
+ | },
+ | "c1"
+ | ]
+ | ]
+ |}
+ |""".stripMargin
+ val request2 =
+ s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | [
+ | "Mailbox/set",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "update": {
+ | "${mailboxId2.serialize}": {
+ | "parentId": "${mailboxId1.serialize}"
+ | }
+ | }
+ | },
+ | "c1"
+ | ]
+ | ]
+ |}
+ |""".stripMargin
+
+ ConcurrentTestRunner.builder()
+ .operation {
+ case (threadNb, step) =>
+ if ((threadNb + step) % 2 == 0) {
+ `with`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(request1)
+ .post
+ } else {
+ `with`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(request2)
+ .post
+ }
+ }
+ .threadCount(2)
+ .operationCount(1)
+ .runSuccessfullyWithin(Duration.ofSeconds(10))
+
+ // Test a list all request
+ val request =
+ s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | ["Mailbox/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": null,
+ | "properties": ["id", "name"]
+ | },
+ | "c2"]
+ | ]
+ |}
+ |""".stripMargin
+
+ val response = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(request)
+ .when
+ .post
+ .`then`
+ .log().ifValidationFails()
+ .statusCode(SC_OK)
+ .contentType(JSON)
+ .extract
+ .body
+ .asString
+
+ assertThatJson(response)
+ .withOptions(new Options(Option.IGNORING_ARRAY_ORDER))
+ .inPath("methodResponses[0][1].list")
+ .isArray
+ .contains(
+ s"""{"id": "${mailboxId1.serialize}", "name":"mailbox1"}""",
+ s"""{"id": "${mailboxId2.serialize}", "name":"mailbox2"}""")
+
+ //Test retrieving these mailboxes specifically
+ val requestSpecific =
+ s"""
+ |{
+ | "using": [ "urn:ietf:params:jmap:core", "urn:ietf:params:jmap:mail" ],
+ | "methodCalls": [
+ | ["Mailbox/get",
+ | {
+ | "accountId": "29883977c13473ae7cb7678ef767cbfbaffc8a44a6e463d971d23a65c1dc4af6",
+ | "ids": ["${mailboxId1.serialize}", "${mailboxId2.serialize}"],
+ | "properties": ["id", "name"]
+ | },
+ | "c2"]
+ | ]
+ |}
+ |""".stripMargin
+
+ val responseSpecific = `given`
+ .header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
+ .body(requestSpecific)
+ .when
+ .post
+ .`then`
+ .log().ifValidationFails()
+ .statusCode(SC_OK)
+ .contentType(JSON)
+ .extract
+ .body
+ .asString
+
+ assertThatJson(responseSpecific)
+ .withOptions(new Options(Option.IGNORING_ARRAY_ORDER))
+ .inPath("methodResponses[0][1].list")
+ .isArray
+ .contains(
+ s"""{"id": "${mailboxId1.serialize}", "name":"mailbox1"}""",
+ s"""{"id": "${mailboxId2.serialize}", "name":"mailbox2"}""")
+
+ /*
+ Data race happens within the hierarchical level upon mailbox rename,
+ and might lead to missing intermediary mailboxes. This is because renaming
+ mailboxes is not concurrent proof.
+
+ This leads to a mailbox having no parentId in JMAP while in IMAP it has some.
+
+ The impact is very low. Note that the mailbox can still be accessed, renamed and moved.
+ */
+ }
+
@Test
def updateShouldAllowParentIdChangeWhenChildMailbox(server: GuiceJamesServer): Unit = {
val mailboxProbe = server.getProbe(classOf[MailboxProbeImpl])
diff --git a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxFactory.scala b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxFactory.scala
index 58c7fd2..4d092c3 100644
--- a/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxFactory.scala
+++ b/server/protocols/jmap-rfc-8621/src/main/scala/org/apache/james/jmap/mail/MailboxFactory.scala
@@ -24,13 +24,14 @@ import org.apache.james.jmap.core.UnsignedInt
import org.apache.james.jmap.mail.MailboxName.MailboxName
import org.apache.james.jmap.utils.quotas.QuotaLoader
import org.apache.james.mailbox._
-import org.apache.james.mailbox.exception.MailboxNameException
+import org.apache.james.mailbox.exception.{MailboxNameException, MailboxNotFoundException}
import org.apache.james.mailbox.model.MailboxACL.EntryKey
import org.apache.james.mailbox.model.{MailboxCounters, MailboxId, MailboxMetaData, MailboxPath, MailboxACL => JavaMailboxACL}
import reactor.core.scala.publisher.SMono
import scala.jdk.CollectionConverters._
import scala.jdk.OptionConverters._
+import scala.util.Try
object MailboxValidation {
private def retrieveMailboxName(mailboxPath: MailboxPath, pathDelimiter: Char): Either[MailboxNameException, MailboxName] =
@@ -178,7 +179,12 @@ class MailboxFactory @Inject() (subscriptionManager: SubscriptionManager, mailbo
val rights: Rights = getRights(resolvedACL)
val namespace: MailboxNamespace = getNamespace(messageManager.getMailboxPath, mailboxSession)
val parentId: Option[MailboxId] = getParentPath(messageManager.getMailboxPath, mailboxSession)
- .map(parentPath => mailboxManager.getMailbox(parentPath, mailboxSession))
+ .flatMap(parentPath => {
+ Try(Some(mailboxManager.getMailbox(parentPath, mailboxSession)))
+ .recover({
+ case _: MailboxNotFoundException => None
+ }).get
+ })
.map(_.getId)
val myRights: MailboxRights = getMyRights(messageManager.getMailboxPath, resolvedACL, mailboxSession)
val isSubscribed: IsSubscribed = retrieveIsSubscribed(messageManager.getMailboxPath, mailboxSession)
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org