ensure storage port never conflicts with client port #765

octonato · 2020-04-06T09:02:48Z

This will fix a flaky test in Lagom lagom/lagom#2782.

As seen in Lagom build failure...

Starting Cassandra on port client port: 33477 storage port 33477 host 127.0.0.1 ...

There is a small chance that the storage port conflicts with the client port as both are picked randomly, but not atomically.

The quick fix is to make sure that the storage port never equals the client port and re-try if necessary.

octonato · 2020-04-06T09:07:02Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

+    // bad luck? try again
+    if (port == usedPort) freePort(usedPort)
+    else port
+  }


Unfortunately, freePort is public API, so instead, I'm adding an overloaded method, but private.

Should we deprecate freePort? I don't think we should have this as public API.

Worse case freePort is not random but the OS insist on using the same over and over here, resulting in infinite loop.

I think it's better we we pick two ports at the same time, before closing them.

There is a SocketUtil in Akka for this, but that is in testkit and we don't want that dependency from the launcher.

👍, I will reserve both together while keeping the sockets open and bound during the selection.

octonato · 2020-04-06T09:14:39Z

This will also fix: lagom/lagom#2688

Same problem, but affecting another Cassandra test.

chbatey

LGTM

chbatey · 2020-04-06T09:23:36Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

+    // bad luck? try again
+    if (port == usedPort) freePort(usedPort)
+    else port
+  }


chbatey · 2020-04-06T09:24:06Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

@@ -82,6 +82,14 @@ object CassandraLauncher {
    port
  }

+  @tailrec
+  private def freePort(usedPort: Int): Int = {
+    val port = freePort()


possibly limit the tries? Tho i can't see why it would keep picking the same port

octonato

I took a different approach. Maybe I'm overthinking it, but here is how it's implemented now.

user calls start with 0 and then randomPort, a random port will be selected and used everywhere. randomPort will tell the truth. That's compatible with previous behavior actually.
user calls start with non-zeo and then randomPort, the user-specified port will be used everywhere. randomPort will return user-specified port, so not random. This differ from previous behavior.
user calls randomPort and then start with 0, a random port will be selected and used everywhere. randomPort will tell the truth. That's compatible with previous behavior actually.
user calls randomPort and then start with non-zero, the user-specified port will be used everywhere. randomPort will tell a lie becasue it will be fixed to a random value that is discarded. That's compatible with previous behavior actually.

octonato · 2020-04-06T11:57:50Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

-      val realHost = host.getOrElse("127.0.0.1")
-      val realPort = if (port == 0) randomPort else port
-      val storagePort = freePort()
+      val realHost = host.getOrElse(DEFAULT_HOST)


I think there was a flaw here. If the user passes a host, we should use it when binding the ports. See below.

octonato · 2020-04-06T11:59:29Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

+    val storageSocket = ServerSocketChannel.open().socket()
+
+    try {
+      clientSocket.bind(new InetSocketAddress(host, requestedPort))


I explicitly bind the requested port, even if it's not 0. We must ensure that this port is free and that storage port won't conflict with it.

octonato · 2020-04-06T12:04:28Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

+        case (Failure(ex1), Failure(ex2)) => throw new RuntimeException(s"Failed to close sockets: client '${ex1.getMessage}', storage '${ex2.getMessage}'" )
+        case (Failure(ex1), _) => throw new RuntimeException(s"Failed to close client-port socket: '${ex1.getMessage}'" )
+        case (_, Failure(ex2)) => throw new RuntimeException(s"Failed to close storage-port socket: '${ex2.getMessage}'" )
+        case (_, _) => // we are fine, all closed


Not so sure if really important to do this.

Maybe

t1.flatMap(_ => t2).get

would be enough (?).

patriknw

LGTM, I guess you want to backport to release-0.x branch so it's included in next 0.10x

octonato · 2020-04-06T13:29:18Z

I guess you want to backport to release-0.x branch so it's included in next 0.10x

Yes, we want to backport it.

octonato · 2020-04-06T14:49:41Z

@chbatey, I have changed this PR quite substantially, you may want to give a second review.

octonato · 2020-04-07T06:08:02Z

Something pretty wrong with my impl:

[info] The CassandraLauncher
Starting Cassandra on port client port: 0 storage port 0 host 127.0.0.1 java version 1.8.0_242-b08
4/06 15:03:42 ERROR[main] o.a.c.s.StartupChecks
      [] - cassandra.jmx.local.port missing from cassandra-env.sh, unable to start local JMX service.
[info] - must support forking *** FAILED *** (45 seconds, 128 milliseconds)
[info]   java.lang.RuntimeException: Cassandra did not start within 45 seconds
[info]   at akka.persistence.cassandra.testkit.CassandraLauncher$.tryConnect$1(CassandraLauncher.scala:409)
[info]   at akka.persistence.cassandra.testkit.CassandraLauncher$.waitForCassandraToListen(CassandraLauncher.scala:413)
[info]   at akka.persistence.cassandra.testkit.CassandraLauncher$.startForked(CassandraLauncher.scala:337)
[info]   at akka.persistence.cassandra.testkit.CassandraLauncher$.start(CassandraLauncher.scala:286)
[info]   at akka.persistence.cassandra.testkit.CassandraLauncher$.start(CassandraLauncher.scala:213)
[info]   at akka.persistence.cassandra.testkit.CassandraLauncherSpec.$anonfun$new$2(CassandraLauncherSpec.scala:52)

Looking...

octonato · 2020-04-07T06:38:59Z

TIL: it turns out that compareAndSet checks for the same instance. 🤦‍♂

A few comments though:

I considered using an Option to make it clear what should be the value when ports were not yet set, ie: None. But it adds too much noise without much value.
I also considered using null. It's ugly, but we never use that value when it's null. I resisted to this one by pure prejudice.
in the end, I chose for a fixed value initialPortsValue

I'm happy to change it if others have different opinions.

patriknw · 2020-04-07T06:55:06Z

cassandra-launcher/src/main/scala/akka/persistence/cassandra/testkit/CassandraLauncher.scala

@@ -68,20 +70,69 @@ object CassandraLauncher {

  private var cassandraDaemon: Option[Closeable] = None

+  private val DEFAULT_HOST = "127.0.0.1"
+
+  private val initialPortsValue = (0, 0)


ensure storage port never conflicts with client port

Manual backport of #765 on release-0.x

octonato commented Apr 6, 2020

View reviewed changes

octonato mentioned this pull request Apr 6, 2020

Failed: CassandraPersistenceSpec javadsl lagom/lagom#2688

Open

chbatey approved these changes Apr 6, 2020

View reviewed changes

ensure storage port never conflicts with client port

e901653

octonato force-pushed the fix-free-port-selection branch from 5e7186e to e901653 Compare April 6, 2020 11:44

octonato commented Apr 6, 2020

View reviewed changes

patriknw approved these changes Apr 6, 2020

View reviewed changes

formatting

c1285b1

octonato added the backport label Apr 6, 2020

typo

ed7072b

use same instance to make compareAndSet work

7017ecd

patriknw reviewed Apr 7, 2020

View reviewed changes

patriknw added this to the 1.0-RC2 milestone Apr 7, 2020

octonato mentioned this pull request Apr 7, 2020

Avoid port collisions in tests, start CassandraLauncher first lagom/lagom#2804

Merged

octonato merged commit 451d657 into akka:master Apr 7, 2020

octonato deleted the fix-free-port-selection branch April 7, 2020 12:26

octonato added a commit to octonato/akka-persistence-cassandra that referenced this pull request Apr 7, 2020

Merge pull request akka#765 from renatocaval/fix-free-port-selection

973d52d

ensure storage port never conflicts with client port

octonato added a commit that referenced this pull request Apr 7, 2020

Merge pull request #769 from renatocaval/release-0.x

4a71513

Manual backport of #765 on release-0.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure storage port never conflicts with client port #765

ensure storage port never conflicts with client port #765

octonato commented Apr 6, 2020 •

edited

octonato Apr 6, 2020 •

edited

chbatey Apr 6, 2020

patriknw Apr 6, 2020

octonato Apr 6, 2020

octonato commented Apr 6, 2020

chbatey left a comment

chbatey Apr 6, 2020

chbatey Apr 6, 2020

octonato left a comment

octonato Apr 6, 2020

octonato Apr 6, 2020

octonato Apr 6, 2020

patriknw left a comment

octonato commented Apr 6, 2020

octonato commented Apr 6, 2020

octonato commented Apr 7, 2020

octonato commented Apr 7, 2020

patriknw Apr 7, 2020

ensure storage port never conflicts with client port #765

ensure storage port never conflicts with client port #765

Conversation

octonato commented Apr 6, 2020 • edited

octonato Apr 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato commented Apr 6, 2020

chbatey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

octonato commented Apr 6, 2020

octonato commented Apr 6, 2020

octonato commented Apr 7, 2020

octonato commented Apr 7, 2020

Choose a reason for hiding this comment

octonato commented Apr 6, 2020 •

edited

octonato Apr 6, 2020 •

edited