Adds a connection pool

[RobertoPrevato]

The code mostly replicates the logic of the Python driver implementation.

[edgedb-clabot]

All committers have signed the CLA.

[1st1]

Overall this looks great! I've done a quick cursory review, will do an in-depth one when the PR is closer to the completion.

[1st1]

Improved the api to instantiate a Pool, adding a PoolOptions interface, and also a static async method to instantiate a pool and have it initialized automatically:

I like this. Let's disable new Pool / pool.initialize() altogether.

[1st1]

The Python driver supports custom connection classes. To support the same in the Node.js driver, we would need to modify the current implementation of Connection and AwaitConnection to use protected constructors, instead of private constructors. How would you like to proceed?

We don't want to support subclassing. The only reason AwaitConnection is even exported is typing, and I think I'll fix that some other way (like exporting only the type via ReturnOf<>).

[1st1]

In the code above, the code execution is not resumed after the CrashTestError is thrown while iterating the async generator.

Why should it be resumed? The exception just crashes the main() function, I don't see what's there to execute? What does specifically fail in that example?

[RobertoPrevato]

In the code above, the code execution is not resumed after the CrashTestError is thrown while iterating the async generator.

Why should it be resumed? The exception just crashes the main() function, I don't see what's there to execute? What does specifically fail in that example?

I looked more into this, and my understanding was wrong. This test wanted to reproduce this one in Python:

    async def test_pool_handles_transaction_exit_in_asyncgen_1(self):
        pool = await self.create_pool(min_size=1, max_size=1)

        async def iterate(con):
            async with con.transaction():
                for record in await con.fetchall("SELECT {1, 2, 3}"):
                    yield record

        class MyException(Exception):
            pass

        with self.assertRaises(MyException):
            async with pool.acquire() as con:
                agen = iterate(con)
                try:
                    async for _ in agen:  # noqa
                        raise MyException()
                finally:
                    await agen.aclose()

        await pool.aclose()

My understanding is that when MyException is raised, code execution resumes in the async generator at the point that yielded, exiting the transaction with a ROLLBACK, thanks to the fact that con.transaction() is an async context manager and its __aexit__ method is called even in this case.

The same doesn't happen in the TypeScript code I wrote: my mistake was thinking that a catch block would catch an exception happened during the async iteration. Instead, only the code inside a finally block is executed and it's the same in Python (I just verified in an example).

Anyway, since the JS driver doesn't have something like the Python driver transaction context manager, there is nothing to test here.

[1st1]

My understanding is that when MyException is raised, code execution resumes in the async generator

The code doesn't magically resume. It's caused by the await agen.aclose() call, that would throw in a cancellation exception into the yield record. The exception would then be handled by async with con.transaction(): in the generator.

Here's an equivalent JS code:

async function *iter() {
    let i = 0;
    try {
        while(1) {
            await new Promise(resolve => setTimeout(resolve, 1000));
            yield i++
        }
    }
    finally {
        console.log('in finally')
        await new Promise((r) => {
            setTimeout(() => {
                console.log('finally done')
            }, 1000)
        })
    }
}

async function main() {
    const it = iter();
    try {
        for await (let i of it) {
            console.log('>>>', i);
            throw new Error;
        }
    }
    finally {
        await it.return()
    }
}

main()

We'll need to think how to implement async with transaction() in javascript so that this semantics is handled properly.

[RobertoPrevato]

Unless I am wrong, the transaction object is not implemented in the JS driver (I'll verify later today).

Earlier today I tried the following piece of code, and the code in the finally block and __aexit__ are executed even without calling the generator .aclose(): why is that? (just out of curiosity)

import asyncio


class AsyncContext:

    async def __aenter__(self):
        return self

    async def __aexit__(self, exc_type, exc, tb):
        # And we get here, too
        print("And here, too")


class MyException(Exception):
    pass


async def iterate(times):
    async with AsyncContext():
        try:
            for x in range(times):
                await asyncio.sleep(0.01)
                yield x
        except Exception as ex:
            # We never get here!!
            print("Not here")
        finally:
            # We get here...
            print("We get here...")


async def main():
    async for item in iterate(10):
        print(item)
        if item > 3:
            raise MyException()


asyncio.run(main())

Produces the output:

0
1
2
3
4
We get here...
And here, too
Traceback (most recent call last):
...

[1st1]

Unless I am wrong, the transaction object is not implemented in the JS driver (I'll verify later today).

Yes, it's still a todo.

Earlier today I tried the following piece of code, and the code in the finally block and aexit are executed even without calling the generator .aclose(): why is that? (just out of curiosity)

Because asyncio uses https://docs.python.org/3/library/sys.html#sys.set_asyncgen_hooks to call aclose() implicitly when an async generator is GCed. I'm not sure if nodejs works the same way, need to test that.

[RobertoPrevato]

@1st1 a few days ago I wanted to propose something like this for transactions (you would find it the edit history of the description of this PR). I then removed this part of text because it's a different topic than connection pooling, and deserves a dedicated PR.

For example, if added to the AwaitConnection class:

async transaction<T>(
  action: (connection: AwaitConnection) => Promise<T>
): Promise<T> {
  let result: T;
  await this.execute("START TRANSACTION");
  try {
    result = await action(this);
    // All done - commit.
    await this.execute("COMMIT");
  } catch (err) {
    await this.execute("ROLLBACK");

    throw err;
  }
  return result;
}

[1st1]

For example, if added to the AwaitConnection class:

Yes, that's the gist of it. There are a few arguments that the function should accept (basically allowing tuning of the transaction parameters), but we can nail that in another PR.

This PR is still in draft, I assume it's not yet ready for a final review? (just asking)

[RobertoPrevato]

The only missing is the documentation: I wrote it but I didn't see it yet on the edgedb-site locally. Because I only looked at it using make html from Sphinx (I didn't know yet how to run edgedb-site when I used Sphinx directly).

[1st1]

This looks great. I've left a few comments, Roberto. Thanks for working on this!

[1st1]

Almost there! Looks good.

[1st1]

Please fix the remaining comments. And don't forget to squash into 1 commit before merging.

Excellent job!