Addressing some thread safety concerns. #81
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
pnomolos
commented
Sep 21, 2016
pnomolos
commented
- Fixed the spec so it's properly multi-threaded. Previously the threads were being joined in the loop, so they were executing in serial.
- Changed the default pool size to 20 so the threaded tests can run
- Snychronized access to running the row function (so functions aren't simultaneously created)
- Synchronized access to the row function cache
- May fix PostgreSQL error » duplicate key value violates unique constraint "pg_proc_proname_args_nsp_index" #72 - given that threading leads to random errors, it may or may not be the case.
- Fixed the spec so it's properly multi-threaded. Previously the threads were being joined in the loop, so they were executing in serial. - Changed the default pool size to 20 so the threaded tests can run - Snychronized access to running the row function (so functions aren't simultaneously created) - Synchronized access to the row function cache
seamusabshere
requested changes
Sep 21, 2016
| end | ||
| ts.each { |t| t.join } |
| row_object = Row.new(selector, setter, options) | ||
| merge_function(row_object).execute(row_object) | ||
| nil | ||
| @row_mutex ||= Mutex.new |
There was a problem hiding this comment.
creating a Mutex with ||= is itself a thread safety concern!
Move mutex definitions to initializer as `||=` is not thread safe Temporarily disable `synchronize` calls.
|
@seamusabshere The build is now broken ;) I'm seeing all sorts of weird stuff locally, the most common being: |
|
@seamusabshere Yeah, that sounds like a good idea. |
|
@seamusabshere You OK if I uncomment the mutexes now? ;) |
|
@seamusabshere I re-added the fixes and we're passing again. I do believe we need the mutexes. |
|
@seamusabshere Mind re-reviewing this? :) |
|
@pnomolos looks good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.