Skip to content
This repository has been archived by the owner on Apr 27, 2020. It is now read-only.

Fixed issue #94 #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed issue #94 #99

wants to merge 2 commits into from

Conversation

Vest
Copy link

@Vest Vest commented Mar 14, 2018

Fixed issue #94 by closing open statements
added out/ (IntelliJ) to .gitignore

added out/ (IntelliJ) to .gitignore

Signed-off-by: Vest <Vest@users.noreply.github.com>
@Vest
Copy link
Author

Vest commented Mar 14, 2018

I think it is possible to improve the fix by using try-with-resources for Kotlin. Unfortunately, the supported JDK should be increased from 1.6 (default for Kotlin) to 1.7 (1.8). E.g.

    compile "org.jetbrains.kotlin:kotlin-stdlib"
    compile "org.jetbrains.kotlin:kotlin-stdlib-jre7"

Can/should I try this option?

@fabianonline
Copy link
Owner

Thanks for this.

Unfortunately(-ish), I had also fixed the bug (and then forgot to push the code).
I think closing the Statement object isn't the best way to proceed: There's just one of those and it's reused all over the place. So closing it could lead to funny problems way down the line.

Since the problem originates from ResultSets being open (which are implicitly closed by calling Statement.close()), my code looks at closing them explicitly after they're used.

As for bumping up the kotlin-stdlib: I'll have a look into that.

@Vest
Copy link
Author

Vest commented Mar 15, 2018

Ok, it is up to you to decide what to do with the PR.
I have found that in some "helper" functions, such as queryInt, you get ResultSet from the connection and sometimes do not close it.
Regarding the statements, I wanted to close the Statements in the Update method, because they were opened in the constructor (init), but I don't see that they were reused. So that is why I am quite safe with my approach.

Stdlib has "use" extension, it might help you to improve the mess with statements & result sets.

If you have reviewed my PR and don't need any change, feel free to close it.

@leijurv
Copy link
Contributor

leijurv commented Mar 28, 2018

@fabianonline please merge =)

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

Successfully merging this pull request may close these issues.

None yet

3 participants