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

ConnectionPool may have issues #51

Closed
datatechnology opened this issue Dec 10, 2018 · 7 comments
Closed

ConnectionPool may have issues #51

datatechnology opened this issue Dec 10, 2018 · 7 comments

Comments

@datatechnology
Copy link

datatechnology commented Dec 10, 2018

objectPool.use(executionContext) { it.sendPreparedStatement(query, values) }, this will mess up the executions when there is a transactions, as you may result in a transaction cross multiple connections but only commit in one

@oshai
Copy link
Contributor

oshai commented Dec 10, 2018

@datatechnology I am not sure what you mean. Can you add an example?
Note that for transactions it is better to use the built-in inTransaction as described in the wiki: https://github.com/jasync-sql/jasync-sql/wiki/Executing-Statements#transactions

@andy-yx-chen
Copy link
Contributor

@oshai , I think @datatechnology means if I have the following code
conn.sendQuery("BEGIN");
conn.sendQuery("delete from sometable where somecondition=true");
conn.sendQuery("insert ...");
conn.sendQuery("Commit");
using the ConnectionPool, it will not work properly, say, the first query could be sent to conn#1, second one could be sent to conn#3 and then when commit, the command could be sent to conn#10, which are totally messed

@andy-yx-chen
Copy link
Contributor

@oshai one suggestion, I know the wiki may help, however, it's always better to make illegal code cannot be compiled instead of having documents to avoid some bad patterned code. One possible fix in this case is to create a PooledConnection class, which inherits from Connection, and have two members, one is the pool and the other is the underlying connection, delegate all sendQuery requests to the underlying connection but connect and close methods, which just override to acquire underlying connection from pool and return the connection to the pool respectively

@oshai
Copy link
Contributor

oshai commented Dec 10, 2018

@andy-yx-chen I think changing the way connection pool works in such a way will make it almost irrelevant to inherit Connection but more act like a connection provider. Also, it will break the simple usage of the pool when you don't need transactions.
So I agree with your general suggestion to make it more clear and fail in compilation if possible, but maybe we can think of another approach. Note that there was already cases that users did the opposite mistake of sending queries to the same connection from multiple threads.

@oshai
Copy link
Contributor

oshai commented Dec 11, 2018

@datatechnology do you still have an issue or can I close it?

@andy-yx-chen
Copy link
Contributor

I will help to create a PR to add a pooled connection that could have transactions cross multiple sendQuery calls

@oshai
Copy link
Contributor

oshai commented Dec 18, 2018

@andy-yx-chen Thanks. So I am closing this issue and let's continue the discussion when you will have a PR.

@oshai oshai closed this as completed Dec 18, 2018
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

No branches or pull requests

3 participants