Add suspend, resume and reconnect methods #64
Conversation
Signed-off-by: Piotr Piotrowski <[email protected]>
Sources/Nats/NatsConnection.swift
Outdated
| if self.state == .connected { | ||
| self.state = .suspended | ||
| try await disconnect() | ||
| self.fire(.disconnected) |
There was a problem hiding this comment.
Should suspended and disconnect be different events? even though suspension technically closes the connection from the clients perspective we are keeping subscription state whereas disconnect should also clear that state?
There was a problem hiding this comment.
We were discussing those with @piotrpio
We should never clear the state of subscriptions and we should always recover them when reconnecting/resuming.
The only difference is that if you disconnect because of suspend, client will not try to reconnect automatically. In all other cases, it will.
There was a problem hiding this comment.
I guess my point is about the event types more than the state. For example when you call close() we're emitting closed as opposed to disconnected
There was a problem hiding this comment.
Yeah, I was wondering weather to add a new event type or not. I was not sure how useful it would be to add a new event type, separate from "disconnected". Now that I think about it, having separate event types would mean you would be able to easily catch and separate instances of disconnects not triggered by the client and potentially take some action. Meanwhile, suspending is always initiated by the library and thus may not need a callback (or have a different one). I'll add the new event type.
Sources/Nats/NatsConnection.swift
Outdated
| } | ||
|
|
||
| func resume() async throws { | ||
| guard self.state == .suspended else { |
There was a problem hiding this comment.
should we make sure state changes are synchronized to avoid races?
There was a problem hiding this comment.
Good point, changed 👍
There was a problem hiding this comment.
do you think checking the state than changing it could also be racy? if we ran them in the event loop we shouldn't need locks or we can lock read-modify if it won't cause deadlocks.
There was a problem hiding this comment.
So you mean managing the state as tasks in event loop right?
There was a problem hiding this comment.
yes, using eventloop.execute { }
There was a problem hiding this comment.
I'm really not sure what would be the best approach here, but possibly it would be safer to execute these on event loop.
I took a stab at this, please take a look at the latest changes.
Signed-off-by: Piotr Piotrowski <[email protected]>
Signed-off-by: Piotr Piotrowski <[email protected]>
Signed-off-by: Piotr Piotrowski <[email protected]>
Sources/Nats/NatsConnection.swift
Outdated
| Task { | ||
| self.state = .closed | ||
| do { | ||
| try await self.disconnect() |
There was a problem hiding this comment.
disconnect is essentially a call to channel.close() and can be done without spawning a Task.
There was a problem hiding this comment.
You're right. I wanted to re-use disconnect method but it does not make sense here.
Sources/Nats/NatsConnection.swift
Outdated
| self.state = .closed | ||
| try await disconnect() | ||
|
|
||
| let eventLoop = self.channel?.eventLoop ?? MultiThreadedEventLoopGroup.currentEventLoop! |
There was a problem hiding this comment.
should we guard these instead? What are the cases channel being nil other than connect() call not being done at this point?
Signed-off-by: Piotr Piotrowski <[email protected]>
Signed-off-by: Piotr Piotrowski [email protected]