Added support for Notifications that use DispatchSource #59
Conversation
…urce to monitor for notifications instead of sleep
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
- Coverage 82.33% 80.29% -2.05%
==========================================
Files 10 10
Lines 1325 1142 -183
Branches 107 0 -107
==========================================
- Hits 1091 917 -174
+ Misses 229 225 -4
+ Partials 5 0 -5
Continue to review full report at Codecov.
|
|
This is really nice, thank you! Personally I wouldn't mind eliminating the old solution for notifications altogether, in favor of using Dispatch. What do you think, @sroebert? |
|
@tanner0101 I think this would remove the need for a pure swift driver. |
Sources/PostgreSQL/Connection.swift
Outdated
| /// - Returns: the dispatch socket to activate | ||
| /// - Throws: if fails to get the socket for the connection | ||
| public func makeListenDispatchSource(toChannel channel: String, queue: DispatchQueue, callback: @escaping (_ note: Notification?, _ err: Error?) -> Void) throws -> DispatchSourceRead { | ||
| guard let sock = Optional.some(PQsocket(self.cConnection)), sock >= 0 |
There was a problem hiding this comment.
This seems a bit strange, why create an optional out of something not optional. Also, there is a callback, I would use this callback in case of an error. This way you can remove throws from this function. So it would change to:
let sock = PQsocket(cConnection)
guard sock >= 0 else {
let error = PostgreSQLError(code: .ioError, reason: "failed to get socket for connection")
callback(nil, error)
return
}There was a problem hiding this comment.
The optional allows using it in a guard statement. I try to put all precondition guards at the top of the function. I'll change.
It can't just return because it returns a non-optional DispatchSource. That's why it throws. Better to explicitly generate an error instead of returning nil.
Sources/PostgreSQL/Connection.swift
Outdated
| guard let sock = Optional.some(PQsocket(self.cConnection)), sock >= 0 | ||
| else { throw PostgreSQLError(code: .ioError, reason: "failed to get socket for connection") } | ||
| let src = DispatchSource.makeReadSource(fileDescriptor: sock, queue: queue) | ||
| src.setEventHandler { [unowned self] in |
There was a problem hiding this comment.
Using unowned self can cause a crash. It might be better not to retain self here, so I would change it to:
src.setEventHandler { [weak self] in
guard let strongSelf = self else {
return
}
// Use strongSelf instead of self
}| try self.execute("LISTEN \(channel)") | ||
| return src | ||
| } | ||
|
|
||
| /// Registers as a listener on a specific notification channel. |
There was a problem hiding this comment.
Considering the new method should be the way to go, I would mark the old function as deprecated.
Sources/PostgreSQL/Connection.swift
Outdated
| /// - Parameter err: Any error while reading the notification. If not nil, the source will have been canceled | ||
| /// - Returns: the dispatch socket to activate | ||
| /// - Throws: if fails to get the socket for the connection | ||
| public func makeListenDispatchSource(toChannel channel: String, queue: DispatchQueue, callback: @escaping (_ note: Notification?, _ err: Error?) -> Void) throws -> DispatchSourceRead { |
There was a problem hiding this comment.
I would simply call this function listen(toChannel:queue:callback:)
There was a problem hiding this comment.
Will do. I'd just copied Apple's naming convention. Better to use a swifty one.
Sources/PostgreSQL/Connection.swift
Outdated
| @@ -152,6 +152,15 @@ public final class Connection: ConnInfoInitializable { | |||
| public let channel: String | |||
| public let payload: String? | |||
|
|
|||
| /// initializer usable without knowledge of CPostgreSQL | |||
| /// required to allow unit testing of classes using Notifications | |||
| public init(pid: Int, channel: String, payload: String?) { | |||
There was a problem hiding this comment.
You do not need to make this init public. If you do @testable import, it should just work. This struct should not be initializable outside of this library.
Sources/PostgreSQL/Connection.swift
Outdated
| /// - Parameter channel: the channel to register for | ||
| /// - Parameter queue: the queue to create the DispatchSource on | ||
| /// - Parameter callback: the callback | ||
| /// - Parameter note: The notification received from the database |
There was a problem hiding this comment.
The note and err parameters cannot be commented like this. It should be part of the callback comments. Also I would write out their full names instead of abbreviations.
There was a problem hiding this comment.
Why? If you comment them that way, option clicking on the listen function shows them in a nested table underneath the callback parameter.
There was a problem hiding this comment.
Ok sorry, didn’t know it worked like that.
|
Nice work, this is much better than the existing function with sleeping. This should simply replace the old one. I added some review comments. Also have a look at the tests, as the changes are only covered 66% |
|
To test the error handling code at 206-207 the test needs to close the connection. Then it crashes in the deinit because that tries to close it, too. This will always crash because cConnection can't be set to nil. Any ideas on how to fix this so it doesn't crash? I see two options:
|
… from method documentation.
…l. Changed callback to use weak instead of unowned.
|
I actually think the And probably the same goes for the |
|
PQfinish is documented as freeing the memory and "must not be used again". That means there is no way to query if the connection is open without a crash. So it comes back to my previous question. Make cConnection nullable or add a flag for the connection state? Generally I'd want to make it optional, but that is an API breaking change. |
|
Ok, yeah that is a tricky one. I would also go for making it optional. I know it is a breaking change, but at the same time, I doubt there will be many (if any) libraries out there, making direct use of this property. What about making this property deprecated, changing it to a var that points to a new optional property, throwing a fatal error if it is nil. |
|
reset() and close() were marked as throws because they called validateConnection(). The connection status isn't checked after the call to PQreset(). Therefore, an error is thrown if the connection is invalid (isn't that the reason reset() is called?) but not if the reset fails. I think reset() should only throw an error if PQStatus() is CONNECTION_BAD after the call to PQreset(). Throwing an error if already closed will fail current tests. Thoughts? Along the same line, I think close should not through an error if already closed. It is throwing an error if isConnection is false. That would mean it would not throw errors anymore, so should close() no longer be marked as throws, or keep it for compatibility? |
|
Yes, that is exactly what I meant. I like the idea of throwing an error is reset does not properly reset. About compatibility, I find it a bit hard, as it is a breaking change, but again, I doubt anyone is using this directly currently. And I don't like the idea of keeping it a throwing function when it does not. So in my opinion we should change it to not throwing. |
…lid. reset() behaves no longer calls validateConnection(), now throws if connection was not properly reset. close() no longer marked throws as it doesn't actually throw anymore. listen() version using sleep deprecated.
|
Any idea what the problem with travis is? I see some message about brew and vapor. |
|
Here are a couple of possible fixes: PowerShell/PowerShell#5062 Either add a homebrew environment variable, or move the location of |
|
can someone merge this if it all good? |
|
@mlilback - Can you update the |
Using a DispatchSource is less wasteful of CPU power as per PostgreSQL documentation.
A parametrized init for Connection.Notification is needed so an application using Notifications does not need to import CPostgreSQL.
Fixed a compiler warning about an unused variable.