Move set_bind to before_serving hook and add close#10
Move set_bind to before_serving hook and add close#10Chaostheorie merged 1 commit intopython-gino:masterfrom
Conversation
Engine set_bind was in a before_first_request hook, and the engine was never closed. Moved the set_bind to a before_serving hook, and added a pop_bind/close to an after_serving hook. This also makes it easier to properly initialize engine in custom cli commands.
|
@spapadim once the tests pass, I would be happy to merge your code. Seems like a good addition. Thank you for contributing |
|
Great, thanks! FWIW, not sure if you mean existing tests (should be ok, but honestly can't recall now if I actually ran them, or cut corners in hurry :), or an additional basic test just for this, eg that sth like following doesn't fail: db.init_app(app)
...
async with app.app_context():
# do sth with dbIf latter, I can give that a stab, at some point.. |
|
Thanks with test I mean the pipeline. It's run on each merge requests / commit. It also checks on code quality and tests |
|
Ah, whoops.. I guess I cut corners in a hurry after all, sorry about that! The I should add a decorator to tests that does those two things (not sure if an app context is needed for tests, but |
|
@fantix could you help out with pytest? |
|
I'm sorry for not picking up further on this. After some local testing it seems you're right about the missing app.startup(). I fixed it in the test and the tests are now working again. I've tested you changes locally and they seem to work. For your chang ehtere are not particular tests required as the changes only affect connection behaviour. |
|
A pre-release (v0.1.1b3) was made to include this change. I'm not too sure about the errors mentioned by @franklx, but this hopefully fixes it. |
3 participants
First off, thanks for writing and maintaining Gino -- nicely done and very useful!
Engine
set_bindwas in abefore_first_request hook, and the engine was never closed. Moved theset_bindto abefore_servinghook, and added apop_bind/closeto anafter_servinghook.This makes it easier to properly initialize engine in custom
clicommands, which typically only have access to an app context, and not to a request context.Furthermore, another person reported that this resolves concurrency problems with hypercorn.
Finally, the proposed change also mirrors what lifecycle hooks other DB extensions use (e.g., Flask-SQLAlchemy, or Gino-Sanic).