ericallam / font_assets

Helps with serving font assets with Rails 3.1
137 stars 79 forks source link

This Gem is not thread safe #26

Open jgwmaxwell opened 10 years ago

jgwmaxwell commented 10 years ago

This gem currently uses a shared ivar which will be overwritten by competing requests in threaded web servers - this PR fixes that by duping and calling out to a private method to eliminate sharing state between threads.

ericallam commented 10 years ago

Is this how other middleware's handle this issue? It looks like it will produce a thread-safe result but is it also creating too many objects by duping every request?

jgwmaxwell commented 10 years ago

Hey, there's basically two choices on this front: you've either go to work without using variables that can be mutated, or instantiate a new handler for each request. Whether there is a standard way to do it is a complex question I think, mainly based around how complex what the middleware does is.

The solution here is to possibly pivot around whether this middleware is interested or not - if it isn't a request that is applicable then just @app.call otherwise dup and process, might be a nice compromise. Ultimately though, thread safety can only be ignored here if one is running Unicorn, which is an increasingly small part of the ecosystem.

ericallam commented 10 years ago

The solution here is to possibly pivot around whether this middleware is interested or not - if it isn't a request that is applicable then just @app.call otherwise dup and process, might be a nice compromise.

I like that idea. Could you update the pull request to do accomplish this? Also, make sure you add specs for anything that is changing, and if you are up for it adding specs to test the thread-safety.

acrogenesis commented 9 years ago

:+1:

PericlesTheo commented 8 years ago

any news regarding this?