Added ping #38
Added ping #38
Conversation
Sources/Benchmark/main.swift
Outdated
| for _ in 0..<numMsgs { | ||
| try! nats.publish(data, subject: "foo") | ||
| } | ||
| try! await nats.flush() | ||
| _ = try! await nats.ping() |
There was a problem hiding this comment.
I think that in client benchmarks we do not care for actual performance and when messages arrives to the server, but the client capacity to send them out. Ping is useful if you want to test the system End to End.
There was a problem hiding this comment.
I don't see the benefit if not measured end to end. maybe I'm missing the point and maybe they are two different benchmarks?
| @@ -0,0 +1,19 @@ | |||
| import NIOConcurrencyHelpers | |||
|
|
|||
| internal class ConcurrentQueue<T> { | |||
There was a problem hiding this comment.
I wonder if DispatchQueue is not more straighforward solution here.
@piotrpio WDYT?
There was a problem hiding this comment.
I think we should avoid GCD unless we have a significant and measurable performance benefit.
There was a problem hiding this comment.
I don't have a strong opinion here. Just prefer using already build concurrent-safe queues instead of making non-concurrent safe, safe by locking.
Especially that most implementations are faster, relying on atomics instead.
There was a problem hiding this comment.
it's locking for memory it should be fine here. other option is to use an actor. I don't know how atomics maybe used here. I don't mind. this won't be on a hot path
| @@ -103,4 +103,13 @@ extension Client { | |||
| return try await connectionHandler.subscribe(subject) | |||
|
|
|||
| } | |||
|
|
|||
| public func ping() async throws -> Duration { | |||
There was a problem hiding this comment.
I thought other clients called it ping as well. is it called rtt in rust client?
There was a problem hiding this comment.
No, in Go it's called Flush with this description:
// Flush will perform a round trip to the server and return when it
// receives the internal reply.
func (nc *Conn) Flush() error {
return nc.FlushTimeout(10 * time.Second)
}
Which I think is confusing for anyone used to what flushing usually does.
| @@ -127,7 +129,6 @@ class ConnectionHandler: ChannelInboundHandler { | |||
| default: | |||
| continuation.resume() | |||
| } | |||
| continue | |||
There was a problem hiding this comment.
otherwise it's ignoring the incoming pong
edit: reverted. now ping queue is ignoring first ping-pong
Sources/NatsSwift/PingCommand.swift
Outdated
| @@ -0,0 +1,24 @@ | |||
| import NIOCore | |||
|
|
|||
| internal class PingCommand { | |||
Added ping command. Should help with #1.