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

Avoid infinite loop calling svp_release() #97

Closed
wants to merge 4 commits into from

Conversation

pmooney
Copy link

@pmooney pmooney commented Mar 10, 2016

We hit a bug in our tests when we rolled back twice to the same savepoint. Although this was our bug we only found out when the test never never completed and used much CPU on a Jenkins box.

@ribasushi
Copy link
Collaborator

Thank you for the contribution. Is there a chance you could add a failing-without-this test case to the end of https://github.com/dbsrgits/dbix-class/blob/master/t/storage/savepoints.t ?

If you do not have time to do this - that is ok: the issue will be eventually resolved either way, but an accompanying test is a hard prerequisite to applying this change.

@pmooney
Copy link
Author

pmooney commented Mar 10, 2016

I was hoping you were not going to say that :)

Give me a day or two and I'll add something.

On 10.03.2016 15:46, Peter Rabbitson wrote:

Thank you for the contribution. Is there a chance you could add a
failing-without-this test case to the end of
https://github.com/dbsrgits/dbix-class/blob/master/t/storage/savepoints.t
[1] ?

If you do not have time to do this - that is ok: the issue will be
eventually resolved either way, but an accompanying test is a hard
prerequisite to applying this change.

Reply to this email directly or view it on GitHub [2].

Links:

[1]
https://github.com/dbsrgits/dbix-class/blob/master/t/storage/savepoints.t
[2]
#97 (comment)

@pmooney
Copy link
Author

pmooney commented Mar 16, 2016

Test added. On the old code it fails, on the new code if passes.

@ribasushi
Copy link
Collaborator

This has been applied with a leaner implementation as 7fd61b1fc. The pop is now localised in one spot, and I also removed the alarm-harness, as the test will not be executed on older versions given it is all in one commit.

Thank you for your work!

@ribasushi ribasushi closed this Mar 30, 2016
@ribasushi
Copy link
Collaborator

ARGH! I didn't wait for the smokes to finish, failures on older sqlite. Will re-push in a bit, and update the commit SHA

@ribasushi
Copy link
Collaborator

Aight, the correct commit is f5f0cb1dd ;)

@ribasushi
Copy link
Collaborator

The position of your test (after another block) managed to uncover and lead to a fix of another bug: 66c817df1, so double-cheers ;)

@pmooney
Copy link
Author

pmooney commented Mar 31, 2016

Glad to be of service :)

The bug in DBIx::Class I fixed highlighted a bug in our own code so it
was actually quite useful ;)

On 30.03.2016 21:37, Peter Rabbitson wrote:

The position of your test (after another block) managed to uncover and
lead to a fix of another bug: 66c817d [1], so double-cheers ;)

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub [2]

Links:

[1] 66c817df1
[2]
#97 (comment)

@ribasushi
Copy link
Collaborator

The fix is now properly in the CPAN index. Sorry it took so long :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants