Closed fabianfett closed 1 year ago
Merging #353 (32aeeae) into main (1516e0c) will increase coverage by
0.63%
. The diff coverage is80.00%
.
@fabianfett No crash so far. 👍
But cancelled queries now return PSQLError(backing: PostgresNIO.PSQLError.(unknown context at $11882746c).Backing)
which is not what I would have expected... Is this maybe some unwanted side effect of the change?
@trasch What did you get before?
What did you get before?
@fabianfett Nothing because it crashed before it could return an error...
But my question was stupid anyway, I didn't properly look at the PSQLError
struct. What I should have asked is: the error code
is now .server
, but there is also .queryCancelled
which is the code that I would have expected if the query gets cancelled by the server.
This information is also contained in serverInfo?[.sqlState]
with the not so helpful number 57014
which I can of course convert to a PostgresError.Code.queryCanceled
(with one l
...), but I'm not sure if this is how it should be? Or maybe do I misunderstand how PSQLError
is intended to be used?
It would be nice if the PSQLError.Backing
had an even infinitesimally useful description
🙂
@fabianfett I've changed the test case for this issue a bit to show two problems that I still have:
CancellationError
- but the query is still running on the connection which blocks. Not sure if this isn't intentional though, I know that PostgreSQL can be somewhat finicky when clients want to kill queries... func testCancelTaskWhileLongRunningQuery() async throws {
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1)
defer { XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) }
let eventLoop = eventLoopGroup.next()
let start = 1
let end = 1000000
let connectionClosesAfterCancel = self.expectation(description: "connection closes after cancel")
Task {
try await withTestConnection(on: eventLoop) { connection -> () in
// Uncommenting this will also crash:
// try await connection.query("SET statement_timeout=1000;", logger: .psqlTest)
let longRunningQuery = Task {
let rows = try await connection.query("SELECT generate_series(\(start), \(end));", logger: .psqlTest)
var counter = 0
do {
for try await element in rows.decode(Int.self, context: .default) {
XCTAssertEqual(element, counter + 1)
counter += 1
}
XCTFail("Expected to get cancelled while reading the query")
} catch is CancellationError {
// Expected
} catch {
XCTFail("Unexpected error: \(error)")
}
XCTAssertFalse(connection.isClosed, "Connection should survive!")
print("longRunningQuery done")
}
Task {
let delay = UInt64(0.2 * 1_000_000_000)
try await Task<Never, Never>.sleep(nanoseconds: delay)
print("Cancel!")
longRunningQuery.cancel()
}
try await longRunningQuery.value
}
connectionClosesAfterCancel.fulfill()
}
await fulfillment(of: [connectionClosesAfterCancel], timeout: 1.0)
}
@trasch Can you give this another spin?!
Can you give this another spin?!
@fabianfett Awesome, short test worked, I will do a longer test run tomorrow
The CI failures here are caused by this issue: https://github.com/apple/swift-nio/pull/2415
In #351 we only landed a simple bandaid that didn't fix the underlying issue. Let's fix the real issue inside the state machine now.
cc @trasch can you very this please?
Fixes #347