prepared statements design problem

[EngineLessCC]

Appending to 157

When you're done with a statement, you can instruct the server to de-allocate it using connection::(async_)close_statement. Note that statement's destructor does not close the statement, since this is an async operation that can fail.

I don't agree with this. As we are using C++, where the point of the language is the use of RAII, this is calling for disaster for many people who assume the default behavior is that the object (statement) is destroyed once it goes out of scope.

Well yes, I understand the concern about async failure, but I prefer at least a synchronous close instead of a leak.

In the end for those who care about async.. there is still the async_close_statement method.

A server resource is no different than a memory resource. But once the statement went out of scope there is no way to close it anymore (leaked) and this is an oversight, especially when connections are long-lived.

[anarthal]

Making the destructor invoke a sync method (that can actually fail) can have even worse consequences:

awaitable<string> do_query(tcp_ssl_connection& conn) {
    results r;
    auto stmt = co_await conn.async_query("SELECT name FROM employees", use_awaitable);
    co_await conn.async_execute(stmt.bind(), r, use_awaitable);
    co_await conn.async_close_statement(stmt, use_awaitable);
    co_return r.at(0).at(0).as_string();
}

If for whatever reason async_execute fails there, you're invoking a sync method in async code, which is really bad.

for those who care about async

You're talking like this is almost no-one, when it's one of the main points of the library.

That said, I'm open to implementing a sync RAII-style guard for statements (e.g. statement_guard) that implements this.

[EngineLessCC]

My thoughts:

Lets assume the connection failed. -> We mark the conn as dead and we throw.

Old Way: nothing, but no leak as the statement closure will be handled by server.

Proposal: In this case, the dtor of stmt which would be calling close_statement should just nop(do nothing).

Q: What about a mysql error/exception?

Old way: we throw and therefore just leak the statement and waste server resources.

Proposal: Thanks to unwind, stmt dtor will be called, which closes the statement, there is nothing that would go wrong here in terms of mysql logic.

Well of course, dtor may throw, bad. But we may just swallow it and not throw? Do we care if cleanup in dtor fails? Dont think so as in worst case the connection has died. (There should be no case where mysql server refuses a statement closure)

You're talking like this is almost no-one, when it's one of the main points of the library.

This is besides the point, do you prefer accidental blowup/DOS of production db (global stmt limit) or reduced performance at the local application level caused by blocking operations?

And if it really has to be async in dtor, or chosen based on if last op was async or not, we could post to an internal connection strand and handle it in the background?

[EngineLessCC]

Three references which all clean up in dtor (and swallow errors):

1: https://github.com/Roslaniec/MariaCpp/blob/master/src/prepared_stmt.cpp#L51 2: https://github.com/mysql/mysql-connector-cpp/blob/trunk/jdbc/driver/mysql_prepared_statement.cpp#L390

3: https://github.com/mariadb-corporation/mariadb-connector-cpp/blob/master/src/ServerSidePreparedStatement.cpp#L38 https://github.com/mariadb-corporation/mariadb-connector-cpp/blob/master/src/util/ServerPrepareResult.cpp#L36

[anarthal]

Sure, in a sync world it's the way to go.

This is besides the point, do you prefer accidental blowup/DOS of production db (global stmt limit) or reduced performance at the local application level caused by blocking operations?

Don't underestimate the impact of calling sync functions in async code. It will completely hang the server thread and make everything unresponsive. And it's an almost-impossible to track bug. This is not something I'm going to assume.

Dispatching in the background is a bad idea because you require that the connection is alive until the operation completes, which you have no way to ensure. So until we have a proper async cleanup mechanism (there's a proposed Boost library that handles this with coroutines), this will only apply to sync users.

Since calling network operations in a destructor is non-conventional in Asio, and it shouldn't apply to async users, I want it to be explicitly opt-in, yet simple to use. I'm thinking of:

void f(tcp_ssl_connection& conn) {
    statement_guard guard = conn.prepare_statement_guarded("SELECT ...");
    results r;
    conn.execute(guard->bind(...), r);
}

With statement_guard wrapping the returned statement and invoking close_statement in the destructor (errors are swallowed).

Would that suit your use case?

[EngineLessCC]

Generally yes, but I have just found out its possible to have async raii: co_resource I would assume this to be added to boost/asio at some point in the future.

That way we can't leak statements nor block threads in async contexts. Can we use that?

async

async_prepare_statement yields a boost::mysql::co_statement_guard and calls async_close_statement on resume. -> resume is called by boost::mysql::co_statement_guards destructor

blocking

prepare_statement (blocking) returns a mysql::statement_guard, which calls close_statement on destructor

example

Example code Output:

main: entry
mysql::connection::async_prepare_statement: entry
mysql::statement::statement: entry
mysql::connection::async_prepare_statement: yield
main: post_create, value : 7
..work... // coroutine holding the statement goes out of scope here!
mysql::connection::async_prepare_statement: post_yield
mysql::connection::async_close_statement: entry, value: 7
mysql::connection::async_prepare_statement: return
mysql::statement::~statement: entry

[anarthal]

That's unfortunately C++20-coroutine-specific, and from what I've read, it looks specific to each coroutine library.

This library targets Asio's universal async model. Async functions are not coroutine or callback based per-se. They all get passed a CompletionToken object, that specifies what they should become into. If you pass the special value boost::asio::use_awaitable, you have C++20 coroutines. If you pass boost::asio::yield_context, you have stackful coroutines. You can pass a callback if you like old style programming. The set of valid completion tokens is not a closed set. This is how you can use the library with C++20 coroutines, while still being compatible with C++11 code.

I think the main problem with C++20 coroutines right now is that you need a lot of boilerplate on top of them to make them useful. There is a library proposed for Boost that implements something very similar to the idea of a co_resource you linked. The author went more Pythonic and called the feature with. If this happens to go into Boost, I will definitely add an async guard class that implements this.

I would still have the C++11 compatibility problem. I can't make it the default return value for async_prepare_statement because it's only compatible with one completion token (in this case use_op, which is the token implemented by the Async library). Otherwise I'd be breaking users of callbacks, stackful coroutines, and so on.

Yeah, doing async with Asio is a headache. It's really powerful, but sometimes you pay too much a price for the generality. I hope that async gets someday widely adopted and we can use safer constructs like the one you describe.

Async's review's taking place on September. I can pass you the dates in case you'd like to participate.

I will try to get the sync guard done for now, and will do the async depending on what happens during the review.