search

discourse / mini_mime

minimal mime type library
MIT License
95 stars 34 forks source link

Make the library fork safe and drop the mutex #50

Closed casperisfine closed 11 months ago

casperisfine commented 11 months ago

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

cc @SamSaffron

SamSaffron commented 11 months ago

I like this change! the concurrency issue I guess is not ideal, but the complexity of resolving it is high and impact extremely low.

Perf wise I assume pread will be faster anyway cause there is one less syscall

I am good to merge this!