Skip to content

Select calls get called twice #97

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

Closed
Killili opened this issue Feb 22, 2017 · 6 comments
Closed

Select calls get called twice #97

Killili opened this issue Feb 22, 2017 · 6 comments

Comments

@Killili
Copy link
Collaborator

Killili commented Feb 22, 2017

Hi,

while implementing my little Logger feature i noticed that i get to many sqlite_step calls, it looks like the _extract methods use reset() at the end and in doing so reset execution_started which leads to another execution of the query when it leaves the scope.
I would fix it but i lack the time right now to test it and i think thats an urgent one to fix.

@zauguin
Copy link
Collaborator

zauguin commented Feb 22, 2017

I see four ways to fix this:

We can delete the reset() calls from extract and extract_single_value. Then the user has to reset on his own, which is what the README says anyway. This would be a semi-breaking change.

To avoid breaking existing code, we could keep the reset and manually set execution_started = true; again afterward.

On the other hand I do not really know why reset sets execution_started = false; in the first place.
I can not think of any reason why somebody would want to explicitly reset a statement and use implicit execution afterward. If we remove this, we have a breaking change too.

We can use execution_started or a new flag to track the reset status of the query.
Then reset() as a public interface could be deprecated and the library would be a bit easier to use.

I will try to prepare a pull request for the last approach. What do you think?

@Killili
Copy link
Collaborator Author

Killili commented Feb 22, 2017

Part of why i didnt fix it after tracking it down is that i could not explain why that change happend and wanted to take a closer look or have someone that knows why to tell me why ;).

And without #96 its hard to write a test to check for that in the future so i guess that's a plus point for #96 already.

As i wrote the initial Prepared statement implementation im all for the "breaking" change that restores the behavior outlined in the README, i dont see it a breaking change its a bugfix for undefined behavior.

I do have some time now and would fix it that way if its ok with you.

Killili added a commit that referenced this issue Feb 22, 2017
- Used getter setter for all uses of execution_started
- removed reset() call from _extract*
- added exception for reexecution of already used statment
@Killili
Copy link
Collaborator Author

Killili commented Feb 22, 2017

#97 will fix this but Travis broke, anyone got a clue about that?

@zauguin
Copy link
Collaborator

zauguin commented Feb 22, 2017

I have restarted the Travis build, but the main problem is, that in the prepared statement test we assume that _extract* resets the statement.

@Killili
Copy link
Collaborator Author

Killili commented Feb 22, 2017

hm thats bad, i will take a look in a moment.

@zauguin
Copy link
Collaborator

zauguin commented Feb 22, 2017

Fixed by #98

@zauguin zauguin closed this as completed Feb 22, 2017
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

2 participants