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

Fixes issue with thread-safety on with-db #236

Merged
merged 1 commit into from
Jun 3, 2014
Merged

Conversation

ceterumnet
Copy link
Contributor

I've added 2 tests to the integration test with-db

The tests:

  1. verify that the version of the with-db I wrote to fix lazyness broke thread safety on the with-db macro.
  2. verify that my new version fixes the thread safety issue.

Let me know what you think of this.

@ceterumnet
Copy link
Contributor Author

Argh - this isn't quite right. I need to tweak the delay factors on the threads...I'll update this shortly.

@ceterumnet
Copy link
Contributor Author

Ok - this request should be good to go.

@immoh
Copy link
Member

immoh commented Jun 1, 2014

Sorry for the delay. I think it fixes the bug but I wrote some improvement suggestions about the code.

@ceterumnet
Copy link
Contributor Author

Ok - I've pushed the changes. However, I wasn't able to eliminate the dynamic var from the lazy relationships. It seems that those are evaluated in a way that the post queries are generated so that they escape the binding. Almost like they are generated before the do-query...

Let me know if you have any additional concerns.

@immoh
Copy link
Member

immoh commented Jun 3, 2014

You're right, post queris are generated before do-query and that's why the binding is not available. Too bad, it wouldn't have simplified it a bit and probably fixed other bugs related to connection escaping.

One more concern: I don't see why transaction should be changed, do you have some example in mind why it wouldn't work? If so, we should probably add a failing test that this change fixes.

@ceterumnet
Copy link
Contributor Author

I can't remember why I changed it ... I've rolled that back and pushed.

Cheers,
Dave

@immoh
Copy link
Member

immoh commented Jun 3, 2014

This is good to go. Thanks.

immoh added a commit that referenced this pull request Jun 3, 2014
Fixes issue with thread-safety on with-db
@immoh immoh merged commit f66404c into korma:master Jun 3, 2014
@mayankag
Copy link

mayankag commented Jun 4, 2014

@immoh can you please release 0.3.2 with this change included. I need to use the with-db feature, and would like to use the thread-safe version.

thank you so much!

@immoh
Copy link
Member

immoh commented Jun 5, 2014

0.3.2 released

@mayankag
Copy link

mayankag commented Jun 5, 2014

thank you!

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

Successfully merging this pull request may close these issues.

3 participants