Repository navigation
Lazily handle setClientInfo/setNetworkTimeout in LazyConnectionDataSourceProxy - #37261
Conversation
sbrannen
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I've requested a few changes and have a question.
Accept and lazily cache calls to setNetworkTimeout even when the provided Executor is null. Since some JDBC driver implementations completely ignore the Executor parameter (or fall back to a default executor), we cannot meaningfully validate or handle a null Executor before the physical connection is obtained.
The Javadoc for java.sql.Connection.setNetworkTimeout(Executor, int) explicitly states that it will throw a java.sql.SQLException if "the executor is null". So, are you claiming that a java.sqlConnection returned from LazyConnectionDataSourceProxy.getConnection(String, String) should not comply with the contract of the JDBC specification? In other words, shouldn't we rather eagerly throw an SQLException for a null Executor?
In any case, please add tests for the null Executor use case.
Once you've addressed these issues I'll take another look.
Cheers,
Sam
|
I also noticed that the Javadoc should be updated. The current wording only mentions auto-commit mode, transaction isolation, and read-only mode:
That sentence is already outdated. It doesn't mention catalog, schema, or holdability, which were added in 6.1.2, and this PR now adds two more deferred properties (client info and network timeout). So, please update the class-level Javadoc by enumerating all of the deferred properties there and explicitly calling out |
I did notice that the spec explicitly says null Executor should throw
If we eagerly throw an To maintain behavioral consistency between lazy and non-lazy usage, I think it's more reasonable to cache the call lazily and defer the exception (if any) to the underlying physical connection. This way, the actual driver decides whether to throw or ignore, and we don't break users who rely on drivers that accept null. What are your thoughts? |
ef9f477 to
92872f1
Compare
|
Thanks for the review. I’ve updated the PR based on all your suggestions, with the exception of the Actually, the behavior of Looking forward to your thoughts. |
sbrannen
left a comment
There was a problem hiding this comment.
Thanks for making the requested changes. This looks a lot better now.
I've requested a few minor changes regarding spacing and diction.
…onnectionDataSourceProxy Extend LazyConnectionInvocationHandler to cache early calls to: - setClientInfo(String, String) - setNetworkTimeout(Executor, int) These methods now defer physical connection acquisition until Statement creation, consistent with existing lazy behavior for autoCommit, readOnly, transactionIsolation, catalog, and schema. Accept and lazily cache calls to setNetworkTimeout even when the provided Executor is null. Since some JDBC driver implementations completely ignore the Executor parameter (or fall back to a default executor), we cannot meaningfully validate or handle a null Executor before the physical connection is obtained. getClientInfo() and getClientInfo(String) remains non-lazy (triggers immediate connection fetch)because it is a read operation whose value cannot be reliably cached due to driver defaults, pooled connection remnants, or external session modifications. setClientInfo(Properties) remains non-lazy. The reason is that JDBC driver implementations are inconsistent. Some treat it as overwrite, others as append/merge. To guarantee behavior identical to non-lazy execution across all driver, we choose not to cache or replay it, avoiding any risk of semantic mismatch. See spring-projectsgh-37258 Closes spring-projectsgh-37261 Signed-off-by: Chengang Guan <guanchengang@qq.com>
See spring-projectsgh-37258 Closes spring-projectsgh-37261 Signed-off-by: Chengang Guan <guanchengang@qq.com>
See spring-projectsgh-37258 Closes spring-projectsgh-37261 Signed-off-by: Chengang Guan <guanchengang@qq.com>
See spring-projectsgh-37258 Closes spring-projectsgh-37261 Signed-off-by: Chengang Guan <guanchengang@qq.com>
|
Fixed via d571c40 |
Extend LazyConnectionInvocationHandler to cache early calls to:
These methods now defer physical connection acquisition until Statement creation, consistent with existing lazy behavior for autoCommit, readOnly, transactionIsolation, catalog, and schema.
Accept and lazily cache calls to setNetworkTimeout even when the provided Executor is null. Since some JDBC driver implementations completely ignore the Executor parameter (or fall back to a default executor), we cannot meaningfully validate or handle a null Executor before the physical connection is obtained.
getClientInfo() and getClientInfo(String) remains non-lazy (triggers immediate connection fetch)because it is a read operation whose value cannot be reliably cached due to driver defaults, pooled connection remnants, or external session modifications.
setClientInfo(Properties) remains non-lazy. The reason is that JDBC driver implementations are inconsistent. Some treat it as overwrite, others as append/merge. To guarantee behavior identical to non-lazy execution across all driver, we choose not to cache or replay it, avoiding any risk of semantic mismatch.
Closes gh-37258