Conform to PooledConnection #43
Conversation
| /// Closes the connection. This method is responsible for properly shutting down | ||
| /// and cleaning up resources associated with the connection. | ||
| public nonisolated func close() { | ||
| Task { |
There was a problem hiding this comment.
@fabianfett just wanted to ping you here to show you the problems that we hit when adopting PooledConnection on an actor. I am curious to hear what you think about this.
There was a problem hiding this comment.
Here is also a few notes I had in context to this.
One reason I have a Task {} in both close and onClose was b/c of a error stating Actor-isolated instance method 'onClose' cannot be used to satisfy nonisolated protocol requirement.
Xcode by default will give you a fix prompting you to Add 'nonisolated' to 'onClose' to make this instance method not isolated to the actor well once you adopt that change you will get a Actor-isolated property 'some prop' can not be referenced from a non-isolated context following that error spiral lead us here.
from my understanding onClose since its part of the actor is isolated but PooledConnection expects that its methods to be nonisolated, thus we hit this conflict.
There was a problem hiding this comment.
I think it is important that close is not async, because the whole point of close is to trigger the close. We don't want to wait for the channel to be closed 100% here.
We can see this as an issue of the ConnectionPool design, or the dreaded we issue of we can't send messages to an actor, without waiting for the response. (which would totally solve the issue here)
Also I want to make explicit, that the run pattern here does not play nicely with the ConnectionPool, as the ConnectionFactory expects the Connection only to be returned, once the connection has been established. So you will need to add a onStarted callback by necessity.
I think I come back to my favorite point about structured concurrency:
- There are multiple sensible ways to structure your concurrency.
- Those different ways may not play nicely with each other.
There was a problem hiding this comment.
@FranzBusch I think the issue is not really the actor here, but the question which concurrency structure do you follow. All our approaches are structured. :)
|
|
||
| private let closePromise: EventLoopPromise<Void> | ||
|
|
||
| public var closeFuture: EventLoopFuture<Void> { |
There was a problem hiding this comment.
this can be marked as nonisolated.
| /// This is useful for performing cleanup or notification tasks. | ||
| public nonisolated func onClose(_ closure: @escaping ((any Error)?) -> Void) { | ||
| Task { | ||
| await self.closeFuture.whenComplete { result in |
There was a problem hiding this comment.
if the closeFuture is marked as nonisolated we don't need a Task and an await here.
| /// Closes the connection. This method is responsible for properly shutting down | ||
| /// and cleaning up resources associated with the connection. | ||
| public nonisolated func close() { | ||
| Task { |
There was a problem hiding this comment.
I think it is important that close is not async, because the whole point of close is to trigger the close. We don't want to wait for the channel to be closed 100% here.
We can see this as an issue of the ConnectionPool design, or the dreaded we issue of we can't send messages to an actor, without waiting for the response. (which would totally solve the issue here)
Also I want to make explicit, that the run pattern here does not play nicely with the ConnectionPool, as the ConnectionFactory expects the Connection only to be returned, once the connection has been established. So you will need to add a onStarted callback by necessity.
I think I come back to my favorite point about structured concurrency:
- There are multiple sensible ways to structure your concurrency.
- Those different ways may not play nicely with each other.
WIP Pooled Connection