search

framework-one / fw1

FW/1 - Framework One - is a lightweight, convention over configuration, MVC application framework for ColdFusion / CFML.
http://framework-one.github.io
Other
374 stars 141 forks source link

Remove locking from framework one global controller retrieval #513

Closed jcberquist closed 4 years ago

jcberquist commented 4 years ago

As it stands right now, every single request that comes in has to acquire the exclusive lock for the global fw1 app controller (i.e. one.cfc itself). There is also a race condition since each request sets its instance of one.cfc into the application scope via the cache (cache.controllers[ componentKey ] = cfc;) and returns the cfc from the cache; it is possible that concurrent requests could overwrite the cache resulting in one of the requests returning the other request's instance of one.cfc. (I do doubt this would have consequences, but it is possible.)

This change pulls the Application controller retrieval out of the locking mechanism, which it does not need since every request has its own instance and the application scope controller cache is not (supposed) to be involved. I retained the autowire call, but not injectFramework( cfc ); since that would result in injecting one.cfc into itself; I could add this back to ensure the behavior remains exactly the same. (Also note that the subsystem bean factory check can be ignored since Application.cfc does not get a subsystem bean factory.)

mjhagen commented 4 years ago

I'm going to run this on my test environment for a while

sneiland commented 4 years ago

I'm going to review this asap however right now its failing the older adobe builds. If those are valid fails then this will need to be reworked.

sneiland commented 4 years ago

After further investigation it looks like the issue may be with the travisci process itself. I'll put this on my list to run on my test for a period.

sneiland commented 4 years ago

@mjhagen Have you encountered any issues with this pull request. I have not seen any in my local testing.

mjhagen commented 4 years ago

Not consciously :) I've only had it on my dev environment, I could run it for some time in production if you want. But tests passed now, right?

sneiland commented 4 years ago

Yup, all tests are passing. Im going to be adding this change to my own prod environment this weekend. I'd like to try run it in a prod like environment for 30 days and if nothing major shows up I'll approve this at the end of that period.