Flag in the close method to abort running queries

[davidmartos96]

Lets users implement use cases such as #394

Other alternative boolean names for the flag:

force
abortRunningQueries
abortRunning
cancelRunningQueries
cancelRunning

[isoos]

I think force, interruptRunning or interruptPending would be a good name for it. I think force is good because it is short, and there is really not much to add to this close method: we either do it nicely or abruptly.

[isoos]

Thanks for the rename! Any idea on how to include a test for this? I think any simple query with sleep and checking of the connection state would do.

[davidmartos96]

@isoos I was actually just trying to create a test, but I'm seeing unexpected results, but might be a misunderstanding on how Postgres manages the current connections.

New test in v3_close_test.dart

test('force close', () async {
      // Get the PID for conn1
      final res = await conn2.execute(
          "SELECT pid FROM pg_stat_activity where application_name = '$conn1Name';");
      final conn1PID = res.first.first as int;

      // ignore: unawaited_futures
      conn1.execute('select pg_sleep(10) from pg_stat_activity;');

      // Let the query start
      await Future.delayed(const Duration(milliseconds: 100));

      await conn1.close(force: true);

      await Future.delayed(const Duration(milliseconds: 1000));

      final afterCloseRes = await conn2
          .execute('SELECT * FROM pg_stat_activity where pid = $conn1PID');
      print(afterCloseRes.map((e) => e.toColumnMap()).toList());
      expect(afterCloseRes, isEmpty);
    });

[davidmartos96]

The Terminate message is sent, and the socket is closed, but the connection still appears in pg_stat_activity

[davidmartos96]

@isoos I added some tests and improved the way force was working with the connection pool. We need to release the pool resources tied to connections when force closing, otherwise, _semaphore.close would wait until the queries finish.

I've added a private class wrapper for the pool resource. Let me know what you think about the approach.

[isoos]

New test in v3_close_test.dart

Could you please create a completely separate test with a new server instance and connections only alive for the time of the test. I am never sure when tests are executed concurrently.

[davidmartos96]

@isoos Would this be ok, and multiple tests inside? Or separate withPostgresServer calls?
As far as I'm aware, different dart test files run concurrently, but test blocks inside a single test file don't.

[isoos]

I was thinking of a separate withPostgresServer, it will give us the proper isolation, regardless of the concurrency otherwise.

[isoos]

I think the non-pool part of this PR are fine, if you separate it, we can have them merged first and iterate on the pool part in a follow-up.

[isoos]

Why would we call release outside the withConnection block?

I think we shouldn't need a separate _PoolConnectionResource, rather track the same state inside _PoolConnection, it seems to me that it should be simpler that way.

[davidmartos96]

It would be called outside when force closing the pool, _semaphore.close() waits for all the resources to finish, so it would be waiting for the queries. So we call release just before calling close.

I mainly created the wrapper because release can't be called twice, then I added the connection because it was in the same scope/context.

Maybe we can keep the wrapper class and remove the connection as a field. What do you think?

[isoos]

I mainly created the wrapper because release can't be called twice, then I added the connection because it was in the same scope/context.

Maybe we can keep the wrapper class and remove the connection as a field. What do you think?

Why not merge the two? I really don't see why you couldn't add it to _PoolConnection, but maybe I should attempt to implement it myself and see how it feels. Hence my suggestion to land the parts that we agree on and treat the pool as a separate goal.

[davidmartos96]

Yes, I'll split the PR.

I originally also thought about adding it to _PgConnection but then I saw that the list of connections is kept separate and I think there is not a 1-1 mapping in the life cycle of the collection and the pool resource. Also, those would be more core changes in the library, that I wouldn't feel comfortable changing too much. But I agree there is room for experimenting there

[davidmartos96]

For splitting the PR, should I make the force flag in the pool noop, or should it throw when provided as true?

[isoos]

I think there is not a 1-1 mapping in the life cycle of the collection and the pool resource.

Yeah, I need to review that part of the code, as I have very little memory on how it is done.

should I make the force flag in the pool noop

Is there any drawback to ignore the flag? I think it should be fine just being a noop, maybe add a TODO to implement it.

[isoos]

Also, I hope it is not too late: keep the pool test in the PR, but make it skip: 'not implemented'

[davidmartos96]

@isoos Removed the pool part, I'll rebase in the other PR once merged