flask: Use a weakref to hold onto the Sentry object when connecting to flask's got_request_exception signal #147

Merged
merged 1 commit into from Jul 14, 2012

Projects

None yet

3 participants

@rslinckx
Contributor

This makes the garbage collector unable to reclaim the memory used by a Flask app.

Test case:

import gc
from flask import Flask
from raven.contrib.flask import Sentry


def create_app():
  app = Flask(__name__)
  Sentry(app)
  return app

for i in range(100):
  create_app()
  gc.collect()

The memory should stay stable, but it keeps growing because the signal stays connected.

@dcramer
Member
dcramer commented Jul 14, 2012

Trying to recall why I changed this to not be a weakref. I think it had something to do with referring to the client.

Anyways, going to merge this in, and if there's a future problem we'll figure it out

@dcramer dcramer merged commit baf1e24 into getsentry:master Jul 14, 2012
@jbardin
jbardin commented Oct 2, 2012

Hi,

So I found out why you probably changed it in the first place.

Basically if the sentry client goes out of scope, the weakref will get removed. If one were to import the create_app example from above, the Sentry client would get configured, but would be removed from the got_request_exception receivers after app is returned. It would be nice if the docs reflected this detail, or weak=False was made an option instead of a default. Since the basic usage doesn't require calling the client ever, it's not obvious that it should still be bound to something.

-jim

@dcramer
Member
dcramer commented Oct 2, 2012

I think we can just bind sentry to app or something so that we don't have to worry about persisting it. It really should be a weakref after all

David Cramer

twitter.com/zeeg
disqus.com/zeeg

On Tuesday, October 2, 2012 at 11:01 AM, James Bardin wrote:

r

@rslinckx
Contributor
rslinckx commented Oct 3, 2012

Maybe the Sentry extensions could register itself in app.extensions['sentry'], although this would prevent configuring multiple Sentry for a single app.

In my code i do:
app.extensions['my-sentry'] = Sentry(app)

this way the client is not garbage collected until the app falls out of scope.

@dcramer
Member
dcramer commented Oct 3, 2012

@rslinckx is app.extensions a standard thing?

@rslinckx
Contributor
rslinckx commented Oct 3, 2012

Yes, as you can see there: http://flask.pocoo.org/docs/api/#flask.Flask.extensions

However as I said, this will prevent multiple Sentry to be registered to a single app (for example if i want to push some exceptions in sentry server A, and in some other cases some exceptions to sentry server B) then i can't do

app = Flask(name)
sentry1=Sentry(app)
sentry2=Sentry(app)

The Flask-SQLAlchemy extensions does store app.extensions['sqlalchemy'] = something, but then it implements the case where you want to connect to different databases itself so you don't need to register multiple flask-sqlalchemy extensions on a single app.

I would say that a first step is to just register sentry in the app.extensions and see later for the multi sentry case...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment