From f67eef65cfc987e476998f71ed7ee265cf0a1f8e Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 2 Aug 2023 21:15:11 +0200 Subject: [PATCH] Make the library fork safe and drop the mutex When forking, file descriptors are inherited and their state shared. In the context of MiniMime this means that the offset of the file opened by RandomAccessDb is shared across processes, so the `seek + read` combo is subject to inter-process race conditions. Of course that file is lazily opened, so assuming most applications don't query MiniMime before fork, it's not a big problem. However when reforking post boot (e.g. https://github.com/Shopify/pitchfork) this becomes an issue. Additionally, even if the file descriptor isn't shared across processes, the file position is still process global requiring a Mutex. By using `pread` instead of `seek + read` we can both make the library fork safe and get rid of the need to synchronize accesses. This also happens to fix an outstanding JRuby issue. Fix: https://github.com/discourse/mini_mime/issues/37 Fix: https://github.com/discourse/mini_mime/pull/38 --- lib/mini_mime.rb | 19 ++++++------------- test/fixtures/custom_content_type_mime.db | 2 +- test/fixtures/custom_ext_mime.db | 2 +- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/lib/mini_mime.rb b/lib/mini_mime.rb index 566a5b1..b6d5dfa 100644 --- a/lib/mini_mime.rb +++ b/lib/mini_mime.rb @@ -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)) end end diff --git a/test/fixtures/custom_content_type_mime.db b/test/fixtures/custom_content_type_mime.db index 0df3883..9e3cb70 100644 --- a/test/fixtures/custom_content_type_mime.db +++ b/test/fixtures/custom_content_type_mime.db @@ -1,2 +1,2 @@ -liquid application/x-liquid 8bit +liquid application/x-liquid 8bit mp4 video/vnd.objectvideo quoted-printable diff --git a/test/fixtures/custom_ext_mime.db b/test/fixtures/custom_ext_mime.db index 2147a75..cc3c2d6 100644 --- a/test/fixtures/custom_ext_mime.db +++ b/test/fixtures/custom_ext_mime.db @@ -1,2 +1,2 @@ -lua application/x-lua 8bit +lua application/x-lua 8bit m4v video/vnd.objectvideo quoted-printable