pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
501 stars 239 forks source link

Only save to redis if session permanent #80

Closed marcuspen closed 7 months ago

marcuspen commented 6 years ago
lucassus commented 2 years ago

Hi! Thank you for the fix ;)

Is there any chance that this PR will be accepted?

Recently, in my application, I noticed that the number of session:* keys is steadily increasing in a very nice, linear way:

image

The culprit is my health_check endpoint.

    @app.route("/health", methods=["GET"])
    def health_check():
        # TODO: In this case, Flask will create a new session for each request,
        #  which will be kept in the storage (e.g. Redis) even if it is empty.
        #  Unfortunately we can't just use `session.permanent = False` within this
        #  request, because flask-session ignores this flag. Therefore, the only
        #  option is to simply clear the session before it hits the storage.
        #  See https://github.com/fengsp/flask-session/pull/80
        session.clear()

        return "OK"

It seems that for this request flask always creates a new session (which is the correct behaviour) but flask-session will keep it the storage even it is empty. The only workaround I have found for this problem is to just clear it before it hits the storage.

Can it be done in a better way?

Lxstr commented 10 months ago

Hi all I'm looking to incorporate #170 but I can't understand exactly how this fix works and what the underlying issue was. Why would you use an impermanent session just within this request? Any clarity would be appreciated.

Lxstr commented 10 months ago

According to https://github.com/christopherpickering/flask-session2/issues/39 this fix doesn't really help anything and it's an inherent issue with all providers?

Lxstr commented 7 months ago

Should now be fixed in 0.6.0