Code review:

- harden relay backoff fields for cross-thread access
- reuse EmptyConnectionListener in backoff test
- extract shared relay-client test fakes
This commit is contained in:
davotoula
2026-06-14 20:36:59 +02:00
parent 312f64dcc3
commit 33a7ef3be5
4 changed files with 76 additions and 64 deletions
@@ -31,6 +31,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocketListener
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebsocketBuilder
import com.vitorpamplona.quartz.utils.Log
import com.vitorpamplona.quartz.utils.TimeUtils
import kotlin.concurrent.Volatile
import kotlin.concurrent.atomics.AtomicBoolean
import kotlin.concurrent.atomics.ExperimentalAtomicApi
import kotlin.coroutines.cancellation.CancellationException
@@ -75,18 +76,22 @@ open class BasicRelayClient(
private var socket: WebSocket? = null
// True if it has received the onOpen call from the socket.
private var isReady: Boolean = false
// @Volatile: written on the serialized socket-callback thread, read from the
// relay-pool/timer thread (see RelayLoadingCursors for the same pattern).
@Volatile private var isReady: Boolean = false
private var usingCompression: Boolean = false
// keeps increasing the delay to connect when errors happen.
// This avoids the constant desire to connect when the server is
// having trouble or offline.
private var lastConnectTentativeInSeconds: Long = 0L // the beginning of time.
private var delayToConnectInSeconds = DELAY_TO_RECONNECT_IN_SECS
// having trouble or offline. @Volatile: read on the pool thread in
// connectAndSyncFiltersIfDisconnected, written on the socket-callback thread.
@Volatile private var lastConnectTentativeInSeconds: Long = 0L // the beginning of time.
@Volatile private var delayToConnectInSeconds = DELAY_TO_RECONNECT_IN_SECS
// when the current connection became ready; used to decide if the
// connection was stable enough to reset the backoff on disconnect.
private var connectedAtInSeconds: Long = 0L
@Volatile private var connectedAtInSeconds: Long = 0L
// Makes sure only one socket is open for each url
private var connectingMutex = AtomicBoolean(false)
@@ -230,6 +235,7 @@ open class BasicRelayClient(
override fun disconnect() {
lastConnectTentativeInSeconds = 0L // this is not an error, so prepare to reconnect as soon as requested.
delayToConnectInSeconds = DELAY_TO_RECONNECT_IN_SECS
connectedAtInSeconds = 0L
socket?.disconnect()
socket = null
isReady = false
@@ -20,11 +20,9 @@
*/
package com.vitorpamplona.quartz.nip01Core.relay.client.single.basic
import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.RelayConnectionListener
import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.EmptyConnectionListener
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocket
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocketListener
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebsocketBuilder
import kotlin.test.Test
import kotlin.test.assertTrue
@@ -38,32 +36,6 @@ import kotlin.test.assertTrue
class BasicRelayClientBackoffTest {
private val url = NormalizedRelayUrl("wss://flaky.example.com")
class FakeWebSocket : WebSocket {
override fun needsReconnect() = false
override fun connect() {}
override fun disconnect() {}
override fun send(msg: String) = true
}
class FakeWebsocketBuilder : WebsocketBuilder {
var connectAttempts = 0
lateinit var lastListener: WebSocketListener
override fun build(
url: NormalizedRelayUrl,
out: WebSocketListener,
): WebSocket {
connectAttempts++
lastListener = out
return FakeWebSocket()
}
}
class NoopListener : RelayConnectionListener
class MutableClock(
var now: Long = 1_000_000L,
)
@@ -99,7 +71,7 @@ class BasicRelayClientBackoffTest {
) = BasicRelayClient(
url = url,
socketBuilder = builder,
listener = NoopListener(),
listener = EmptyConnectionListener,
nowInSeconds = { clock.now },
)
@@ -167,7 +139,7 @@ class BasicRelayClientBackoffTest {
runTicks(client, builder, clock, totalSeconds = 15 * 60) { listener ->
listener.onOpen(50, false)
}
check(client.isConnected()) { "Test setup: relay should have reconnected and stayed up" }
assertTrue(client.isConnected(), "Test setup: relay should have reconnected and stayed up")
clock.now += 10 * 60
builder.lastListener.onClosed(1000, "server restart")
@@ -23,36 +23,11 @@ package com.vitorpamplona.quartz.nip01Core.relay.client.single.basic
import com.vitorpamplona.quartz.nip01Core.relay.client.listeners.RelayConnectionListener
import com.vitorpamplona.quartz.nip01Core.relay.client.single.IRelayClient
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocket
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocketListener
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebsocketBuilder
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
class BasicRelayClientTest {
private class FakeWebSocket : WebSocket {
override fun needsReconnect() = false
override fun connect() {}
override fun disconnect() {}
override fun send(msg: String) = true
}
private class FakeWebsocketBuilder : WebsocketBuilder {
var capturedListener: WebSocketListener? = null
override fun build(
url: NormalizedRelayUrl,
out: WebSocketListener,
): WebSocket {
capturedListener = out
return FakeWebSocket()
}
}
private class RecordingConnectionListener : RelayConnectionListener {
val cannotConnectMessages = mutableListOf<String>()
@@ -81,9 +56,7 @@ class BasicRelayClientTest {
listener,
)
client.connect()
val socketListener = builder.capturedListener
assertNotNull(socketListener)
return Harness(socketListener, listener)
return Harness(builder.lastListener, listener)
}
@Test
@@ -0,0 +1,61 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.quartz.nip01Core.relay.client.single.basic
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocket
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebSocketListener
import com.vitorpamplona.quartz.nip01Core.relay.sockets.WebsocketBuilder
/**
* No-op [WebSocket] for tests. connect/disconnect/send do nothing; the relay's
* behavior is driven by invoking the [WebSocketListener] callbacks captured by
* [FakeWebsocketBuilder].
*/
class FakeWebSocket : WebSocket {
override fun needsReconnect() = false
override fun connect() {}
override fun disconnect() {}
override fun send(msg: String) = true
}
/**
* [WebsocketBuilder] that records how many sockets were built ([connectAttempts])
* and exposes the most recently captured [WebSocketListener] so a test can drive
* onOpen/onFailure/onClosed. [lastListener] is set on every [build]; read it only
* after the client has attempted to connect.
*/
class FakeWebsocketBuilder : WebsocketBuilder {
var connectAttempts = 0
lateinit var lastListener: WebSocketListener
override fun build(
url: NormalizedRelayUrl,
out: WebSocketListener,
): WebSocket {
connectAttempts++
lastListener = out
return FakeWebSocket()
}
}