Make gae_mini_profiler thread safe #30

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

Contributor
cdman commented Jun 20, 2012

These small changes make gae_mini_profiler threadsafe (ie. usable with the new Python27 runtime + threadsafe enabled).

Contributor
cdman commented Jul 27, 2012

The request_id needs to be thread_local since it is used outside of the lock. The middleware needs to be synchronized since it uses a lot of instance variables which could be corrupted if multiple threads set them concurrently.

aburgel commented Aug 7, 2012

I've also run into threading issues with gae_mini_profiler. But I'm not sure if this patch really fixes it. It certainly helps now that request_id is thread local. But the locking effectively serializes all requests. Maybe since not every request is profiled, this is ok performance-wise, but wouldn't it be better to store all this state in the environ dict?

I'm not too familiar with wsgi, but it looks like environ contains per-request data. I will take a look into this and post a patch if it works.

Contributor
kamens commented Aug 7, 2012

@aburgel Give me a couple days to get this repo updated w/ our latest from the Khan Academy codebase -- we have our own multithreading patch. Will update here in the near future, and you can judge if you like the latest.

Contributor
cdman commented Aug 7, 2012

@aburgel - true, this patch serializes profiled requests. Non-profiled requests go trough without locking, so this should have a small impact on production (ie. only the percent you want to be profiled gets serialized). I feel that the locking is needed in addition to the thread-local stuff because the recorder instance fetched in wrapped_appstats_app is used later (on the line RequestStats(request_id, environ, self).store()) and without locking an other thread can indirectly change it (ie. if a different thread starts a request I think that the recorder state might be compromised).

@kamens - I'm looking forward to your patch, hopefully it will give a more elegant solution to the problem.

Any updates on this bug?

@irvingpop irvingpop referenced this pull request in kamalgill/flask-appengine-template Dec 22, 2012
Closed

Problems with gae-mini-profiler #36

Contributor
kamens commented Jan 4, 2013

The latest version should now play nice w/ threadsafe:true.

The new example app running at http://mini-profiler.appspot.com/ is running in multithreaded mode.

The latest version changed the way configurations are overridden (choosing to follow the appengine_config model). The updated instructions at https://github.com/kamens/gae_mini_profiler/blob/master/README.md#start may be needed for those making the switch.

Thx for all the patience!

@kamens kamens closed this Jan 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment