Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

r2dbc-mysql queueing execution of queries #343

Conversation

FutureGadget
Copy link
Contributor

This is my suggested fix for issue #255 (comment).

Since in reactor environment, multiple threads can access to the same connection unintendedly, I added a queue per JasyncClientConnection.

@oshai
Copy link
Contributor

oshai commented Dec 12, 2022

Thanks for the PR!

Can you give some details on the suggested fix?
The test is a really good start.
Having a queue for queries (with scheduler) seems like not the optimal approach.
I am not sure how the driver expected to handle simultaneous queries on the same connection? multiplexing?
Jasync doesn't support multiplexing at the moment, but it seems transactions are not directly related to that.

@FutureGadget
Copy link
Contributor Author

FutureGadget commented Dec 12, 2022

Thanks for the PR!

Can you give some details on the suggested fix? The test is a really good start. Having a queue for queries (with scheduler) seems like not the optimal approach. I am not sure how the driver expected to handle simultaneous queries on the same connection? multiplexing? Jasync doesn't support multiplexing at the moment, but it seems transactions are not directly related to that.

@oshai, thank you for taking a look on this PR.
What I tried to fix with this PR was an "unintended" conflict of query on transaction rollback.

What I mean by "unintended" is when you are in a transaction, cancelling the subscription of ongoing query publisher can cause an asynchronous ROLLBACK query to be executed using the same connection.

This behavior is from the following code. The transactional method uses Mono.usingWhen(...,asyncCancel), and the asyncCancel callback function parameter will make ROLLBACK happen on subscription cancellation.

Mono.usingWhen(Mono.just(it), 
    ignore -> mono,
    this.transactionManager::commit, 
    (res, err) -> Mono.empty(), 
    this.transactionManager::rollback
)

(https://github.com/spring-projects/spring-framework/blob/main/spring-tx/src/main/java/org/springframework/transaction/reactive/TransactionalOperatorImpl.java#L81)

It is "unintended" because a user cannot control the ROLLBACK query to be executed after the ongoing query is finished. As the jasync driver validates the connection before sending any query to the database, the above behavior will make jasync driver throw an exception since the ROLLBACK query cannot be executed simultaneously while the other query being executed using the same connection.

This is critical because when this type of error occurs, resource cleanup fails, which means the connection is not returned to the connection pool and the number of idle connections in the pool are only decreased.

In this PR, I added QueueingExecutionJasyncConnectionAdapter, which adapts MySQLConnection to use a ArrayBlockingQueue. The number of simultaneous queries should be at most 2 (the original first query and the following rollback query), so the size of the queue, which is 256, must be enough.

I didn't intend to introduce a behavior which is similar to query multiplexing with this PR.
Therefore, simultaneous queries using a single connection should be best avoided as it was before this PR.

@oshai
Copy link
Contributor

oshai commented Dec 12, 2022

I have few ideas/options to make it simple:

  • on rollback, just close the connection (which I think will implicitly rollback the current transaction)
  • use queryPromiseReference and chain the cancellation to it (if exists)

What do you think?

@FutureGadget
Copy link
Contributor Author

I have few ideas/options to make it simple:

  • on rollback, just close the connection (which I think will implicitly rollback the current transaction)
  • use queryPromiseReference and chain the cancellation to it (if exists)

What do you think?

Thank you for better suggestions.
I've just tried the second one (chaining query promise) and it worked like a charm.
Please review : #345
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants