Skip to content
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

This Gem is not thread safe #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jgwmaxwell
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link

👍

@PericlesTheo
Copy link

any news regarding this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants