[Fix] Query Hangs if Connection is Closed

[MahdiBM]

See the test for how to trigger this.

[MahdiBM]

Cancelled the integration tests CI manually, since they wouldn't have finished.

Side note @gwynne: probably good to have a time limit in CIs.

[gwynne]

Side note @gwynne: probably good to have a time limit in CIs.

The default timeout is 1 hour, IIRC.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 55.28%. Comparing base (7b621c1) to head (ceaa688).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   54.98%   55.28%   +0.30%     
==========================================
  Files         127      127              
  Lines       10155    10161       +6     
==========================================
+ Hits         5584     5618      +34     
+ Misses       4571     4543      -28     

Files	Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift	48.57% <75.00%> (+6.32%)	⬆️

... and 3 files with indirect coverage changes

[fabianfett]

Great catch! We really need to make sure we cover all cases by adding unit tests for each of them!

[fabianfett]

Sorry used wrong button before :)

[fabianfett]

the test I added as an example hangs 100% of time for me.

[MahdiBM]

the test I added as an example hangs 100% of time for me.

Hmm you're right. I thought i tested that properly and it turned out like i said, but apparently not.
I did try to remove the loop like you did with just closing the channel beforehand, but i thought that didn't work out to get the hang.

I was likely testing with my fixes in there, and not reverted.

[MahdiBM]

ahh hmm CI is failing with Fatal error: SWIFT TASK CONTINUATION MISUSE: listen(_:) tried to resume its continuation more than once, throwing ioOnClosedChannel! need to investigate.

[MahdiBM]

I reckon I didn't add a unit test for "each" modified function. Should I?

[fabianfett]

I reckon I didn't add a unit test for "each" modified function. Should I?

Yes, please!

[fabianfett]

What connection state do you want to test here? In which connection state does this happen? Can we mock this explicitly and not by accident?

[MahdiBM]

The test is consistent, it only waits 3 seconds to make sure the listener has started listening.

It tests that if there is a listen in process, and then the connection is closed, the listener sequence needs to fail.

[fabianfett]

ah! in that case we should wait for the client messages on the channel instead of using a timer.

See here how it is done:

postgres-nio/Tests/PostgresNIOTests/New/PostgresConnectionTests.swift

Lines 83 to 96 in 7b621c1

	func testSimpleListen() async throws {
	let (connection, channel) = try await self.makeTestConnectionWithAsyncTestingChannel()
	try await withThrowingTaskGroup(of: Void.self) { taskGroup in
	taskGroup.addTask {
	let events = try await connection.listen("foo")
	for try await event in events {
	XCTAssertEqual(event.payload, "wooohooo")
	break
	}
	}
	let listenMessage = try await channel.waitForUnpreparedRequest()
	XCTAssertEqual(listenMessage.parse.query, #"LISTEN "foo";"#)

[MahdiBM]

Thanks for the clues, I did this in the new test.
We no longer need to change that fatalError("Invalid state: \(self.state)"), it appears that was triggered as a result of my bad test.

@fabianfett i think i no longer need to make another PR out of this?

[MahdiBM]

@fabianfett I think this PR's changes are good, but I noticed this results in a fatal error regardless of my changes:

    func testAddListenerFailsIfConnectionIsClosed() async throws {
        let (connection, channel) = try await self.makeTestConnectionWithAsyncTestingChannel()

        connection.addListener(channel: "example") { context, notification in
            XCTFail("Did not expect to receive")
        }

        try await connection.close()

        XCTAssertEqual(channel.isActive, false)
    }

[MahdiBM]

Using .close() instead of .closeGracefully() here because .closeGracefully() will wait it out till infinity and beyond, apparently because it's "graceful" and the listener is still listening.

[fabianfett]

Great patch! Thanks for pushing through!