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

add config.threadsafe! #770

Closed
wants to merge 2 commits into from
Closed

add config.threadsafe! #770

wants to merge 2 commits into from

Conversation

marutosi
Copy link
Contributor

No description provided.

@tessi
Copy link
Collaborator

tessi commented Dec 20, 2013

As far as I know, our code could not be farther away from being thread safe. Thus, we deploy everything using multiple processed (and not threads) anyway.

After reading through the blog post from tenderlove and this github pull request, I think it is a good idea to set config.threadfsafe! (and then removing that setting once we are running on rails 4).

However, this change also affects code loading and we should definitely give this a good test before merging (I do not feel confident enough to foresee everything this might affect).

Toshi MARUYAMA added 2 commits January 29, 2014 01:54
It is too hard to run travis on forked branches.
It disturbs pull request process.
@linki
Copy link
Contributor

linki commented Feb 6, 2014

since we are running our deployments with multiple processes, it should probably be ok to set threadsafe! but i don't think it's a good idea. i think the opposite would be a more appropriate solution: since our code is not thread-safe we should mark it as such. especially in preparation for rails 4 when it becomes the default and we might forget about it then.

@mfrister
Copy link
Contributor

As far as I understand the blog post, this disables class reloading, which I assume it also does in development mode.

Since OpenProject is not thread-safe, I'd find it a bit strange to call a method saying so. If Rails 4 does this by default, we could figure out what to do then. We still wouldn't give anyone the impression of being thread-safe while we're not.

Closing the pull request for now, if we nevertheless want to do this, we should create a PR not changing travis.yml.

@mfrister mfrister closed this Feb 26, 2014
@marutosi marutosi deleted the config.threadsafe branch May 26, 2014 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants