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

In BatchExecutor, close each statement right after it is executed. #1110

Merged
merged 2 commits into from Oct 15, 2017

Conversation

Projects
None yet
3 participants
@vlastimil-dolejs
Copy link
Contributor

vlastimil-dolejs commented Oct 5, 2017

fix for #1109

@@ -149,9 +150,6 @@ public int doUpdate(MappedStatement ms, Object parameterObject) throws SQLExcept
}
return results;
} finally {
for (Statement stmt : statementList) {

This comment has been minimized.

@kazuki43zoo

kazuki43zoo Oct 5, 2017

Member

In this changes, If an exception occurred during statement execution, I think there is a case that statement does not close.

This comment has been minimized.

@vlastimil-dolejs

vlastimil-dolejs Oct 5, 2017

Author Contributor

Thank you for review - you are right. I will improve the code.

@harawata harawata changed the title statement is closed immediately after execution In BatchExecutor, close each statement right after it is executed. Oct 15, 2017

@harawata harawata merged commit e72aa99 into mybatis:master Oct 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 15, 2017

@vlastimil-dolejs Thank you for the PR!

@kazuki43zoo Any idea for testing this? I couldn't think of a good one.

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 21, 2017

Hi @harawata, it's difficult .. :(

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 21, 2017

May possible if use the mock mechanism like Mockito...

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 21, 2017

@harawata @vlastimil-dolejs I have one question.
By this changes, probably the Statement.close() is called twice. Is no problem as JDBC specification?

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 21, 2017

(Probably) Calling the close method at twice is no problem because the JDBC 4.2 specification("13.1.4 Closing Statement Objects") says as follows:
However, I think changing as calling only once is a good code.

Once a Statement has been closed, any attempt to access any of its methods with the exception of the isClosed or close methods will result in a SQLException being thrown.

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 21, 2017

@kazuki43zoo ,

May possible if use the mock mechanism like Mockito...

OK. Let's just leave it for now.
I'll add a comment later.

However, I think changing as calling only once is a good code.

+1

@vlastimil-dolejs

This comment has been minimized.

Copy link
Contributor Author

vlastimil-dolejs commented Oct 23, 2017

@kazuki43zoo
JavaDoc of Statement.close() says:

Calling the method close on a Statement object that is already closed has no effect.

So it's safe to call it twice. I think that code that ensures the close() method is called exactly once will add a lot of complexity.

harawata added a commit that referenced this pull request Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment