-
Notifications
You must be signed in to change notification settings - Fork 34
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
Make the library fork safe and drop the mutex #50
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,8 +50,6 @@ def binary? | |
end | ||
|
||
class Db | ||
LOCK = Mutex.new | ||
|
||
def self.lookup_by_filename(filename) | ||
extension = File.extname(filename) | ||
return if extension.empty? | ||
|
@@ -60,18 +58,14 @@ def self.lookup_by_filename(filename) | |
end | ||
|
||
def self.lookup_by_extension(extension) | ||
LOCK.synchronize do | ||
@db ||= new | ||
@db.lookup_by_extension(extension) || | ||
@db.lookup_by_extension(extension.downcase) | ||
end | ||
@db ||= new | ||
@db.lookup_by_extension(extension) || | ||
@db.lookup_by_extension(extension.downcase) | ||
end | ||
|
||
def self.lookup_by_content_type(content_type) | ||
LOCK.synchronize do | ||
@db ||= new | ||
@db.lookup_by_content_type(content_type) | ||
end | ||
@db ||= new | ||
@db.lookup_by_content_type(content_type) | ||
end | ||
|
||
class Cache | ||
|
@@ -146,8 +140,7 @@ def lookup_uncached(val) | |
end | ||
|
||
def resolve(row) | ||
@file.seek(row * @row_length) | ||
Info.new(@file.readline) | ||
Info.new(@file.pread(@row_length, row * @row_length).force_encoding(Encoding::UTF_8)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum, I really sorry, I realized that some platforms (e.g. Windows) don't have I'll submit another PR to shim it for these. |
||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
liquid application/x-liquid 8bit | ||
liquid application/x-liquid 8bit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This padding was missing. |
||
mp4 video/vnd.objectvideo quoted-printable |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
lua application/x-lua 8bit | ||
lua application/x-lua 8bit | ||
m4v video/vnd.objectvideo quoted-printable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small race condition here, that could lead to more than one DB to be instantiated, but only one will be kept so not sure if it's a big deal.
We could eagerly instantiate the db, and reset it when the paths are changed, but that would change the semantic a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me, but I guess there could be a performance impact if a lot of threads try to init the DB at the same time (e.g. because they're all executing the same startup code in lockstep). A possible alternative to avoid multiple initialization could be the DCL-like idiom
@db || MUTEX.synchronize { @db ||= new }
. (I can submit a PR if you think that would be a good idea.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good idea. Not sure why I didn't think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@casperisfine PR here: #56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, note that I'm not maintainer though, it's up to @SamSaffron to merge this or not.