Fix for Issue 23 #45
Fix for Issue 23 #45
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 73.75% 74.21% +0.46%
==========================================
Files 3 3
Lines 461 512 +51
==========================================
+ Hits 340 380 +40
- Misses 121 132 +11
Continue to review full report at Codecov.
|
| } | ||
| PQclear(result) | ||
| return errorMessage | ||
| setState(.idle) |
There was a problem hiding this comment.
Since setState(.idle) is the first thing done regardless of which path we take, it would be clearer if there was a single call, moved before the if status.
| @@ -484,6 +511,7 @@ public class PostgreSQLConnection: Connection { | |||
| } | |||
|
|
|||
| PQclear(result) | |||
| setState(.idle) | |||
There was a problem hiding this comment.
Same comment as above - could move single setState(.idle) to above the if status
| currentResultFetcher?.hasMoreRows = false | ||
| unlockStateLock() | ||
| clearResult(nil, connection: self) | ||
| lockStateLock() |
There was a problem hiding this comment.
I think there's a potential problem here. Another thread could get in to setupForRunningQuery() in between the unlock and lock calls. The code has to be like this because the lock used is not recursive (and would otherwise result in deadlock). I wonder if we could be using an NSRecusiveLock instead?
| import Foundation | ||
|
|
||
| enum ConnectionState { |
There was a problem hiding this comment.
This might be better as a nested enum inside PostgreSQLConnection, called State.
| @@ -342,6 +356,11 @@ public class PostgreSQLConnection: Connection { | |||
| return | |||
| } | |||
|
|
|||
| if let error = setUpForRunningQuery() { | |||
There was a problem hiding this comment.
Naming this error implies it's an Error, which it isn't.
| unlockStateLock() | ||
| } | ||
|
|
||
| func setUpForRunningQuery() -> String? { |
There was a problem hiding this comment.
This is weird, a setup function which returns an optional String? Why not just make it throw?
|
I will defer these requested changes to the original author - @irar2 |
These changes also help workaround issue 24.