in memory sqlite db doesn't work after this change.

Hi @jkaluza , this is for FACTORY-5167, could you help to give a brief review?

I'll do more testing on this to ensure it doesn't break anything.

Patch is updated and ready for review.

This seems pretty straight forward. It looks good to me.

Seems this is a wide change, can some of you also review? @jkaluza @cqi

For running tests in an in-memory sqlite db, pr[1] could be helpful hopefully.

[1] https://pagure.io/fm-orchestrator/pull-request/1511

+ # Create per-handler db session instead of using the global 'db.session',
+ # this is to avoid caching lots of records in the global 'db.session' object
+ self.db_session = self._create_db_session()

This change should not be helpful to achieve the goal to avoid caching lots of records.

With this change, every handler will have its own db session object and all the resources, which are not released explicitly, will remain in the object. Meanwhile, when process an event, a handler object is initialized and can_handle is called to determine whether the event can be handled by that handler. That means the db session object is created even if it is not used.

I would suggest another idea to implement per-handler db session, FYI.

The core concepts used here are

• SQLAlchemy scoped_session is a thread-local session[1].
• Session.remove is used to release resources managed by a session object.

Implementation steps:

1. Call scoped_session to create a global db session object.
2. Modify BaseHandler.handle to accept the db session object as an argument.
3. Pass the db session object to BaseHandler.handle in FreshmakerConsumer.process_event.
4. After the call of BaseHandler.handle, whatever success or failure, call Session.remove to release all the resources explicilty.

A pseudo-code could be:

engine_opts = {
    "configuration": {"sqlalchemy.url": conf.sqlalchemy_database_uri},
}
engine = sqlalchemy.engine_from_config(**engine_opts)
session_factory = sessionmaker(bind=engine)
db_session = scoped_session(session_factory)
try:
    further_work = handler.handle(msg, db_session) or []
except:
    # handle the exception
finally:
    db_session.remove()

Two major benefits are:

• Only one SQLAlchemy database session is created per-handler.
• All the resources managed by the session as well as the session object itself could be released and removed from memory as earlier as possible just after the BaseHandler.handle call.

[1] https://docs.sqlalchemy.org/en/13/orm/contextual.html#unitofwork-contextual

This change should not be helpful to achieve the goal to avoid caching lots of records.
With this change, every handler will have its own db session object and all the resources, which are not released explicitly, will remain in the object.

I think the handler object should be garbage collected by python after process_event, and thus the handler.db_session is released as well.

Pass the db session object to BaseHandler.handle in FreshmakerConsumer.process_event.
After the call of BaseHandler.handle, whatever success or failure, call Session.remove to release all the resources explicilty.

I'm not sure about this, actually if this works, we don't need to create another db session locally as you mentioned, we just need to call remove against the global db.session after handler.handle.

Hi @jkaluza , do you have any idea? is there a way for us to check what is cached in a db session?

I'd like to hold this before we figure out a way to check and verify what are cached before and after the change. Also, it may be possible to just add db.session.remove() after handler.handle() (thanks for @cqi's reminder), but I need an approach to test that.

@qwan

Can you elaborate why do you so care about what and how many things are cached in a db session?

My understand of the original issue, what and how many things cached in a db session is not the key point. Instead, the key point is resources related to a handler run and hold by a db session should be released to ensure they do not remain in memory for a longer time.

It would be good if you can test and figure out the details of what and how many things are cached, but I don't think it is a major factor to consider the implementation solution.

it may be possible to just add db.session.remove() after handler.handle()

This is good point to consider. However, from the frontend and backend separation point of view, calling db.session.remove() should not be helpful. I don't mean to change the scope of the original issue. I'm happy to see a good solution to make the source code flexible enough to be able to fit future requirements.

@qwan
Can you elaborate why do you so care about what and how many things are cached in a db session?

There are more details in FACTORY-5167.

My understand of the original issue, what and how many things cached in a db session is not the key point. Instead, the key point is resources related to a handler run and hold by a db session should be released to ensure they do not remain in memory for a longer time.

As the issue says, the key point is there are too much ArtifactBuild objects cached in the db.session. Though I'm currently have no idea how it happens.

It would be good if you can test and figure out the details of what and how many things are cached, but I don't think it is a major factor to consider the implementation solution.

it may be possible to just add db.session.remove() after handler.handle()

This is good point to consider. However, from the frontend and backend separation point of view, calling db.session.remove() should not be helpful. I don't mean to change the scope of the original issue. I'm happy to see a good solution to make the source code flexible enough to be able to fit future requirements.

Actually I was wrong, because the current code already performs db.session.remove as we wrapped self.process_event(msg) with flask appcontext, and flask_sqlalchemy do that for us automatically while teardown the appcontext. That means if we see memory issues with the original code, session.remove doesn't help on the case.

I see. It's interesting that what I observed is the session object is actually closed and removed properly when existing the app context. Let's see what you find out by your research and see whether it is related to the db session or not.

FYI, objgraph is useful for detecting memory leaks.

After some investigation and talked with Jan, it is probably not the db session that causing the memory issue, so I'm going to abandon this PR. @jkaluza said it may be the lightblue class causing the memory issue, that's a clue for us to troubleshoot.