request middleware not running when registered in a method since 22.9.0 #2699
Comments
|
When I first looked at this earlier I was super shocked and quite disturbed to hear this.
So I think we need to clarify a few things. Middleware added in a listener will work and runYour example code draws an incorrect conclusion here: @app.before_server_start
async def before_server_start_listener(app, loop):
def this_fails_in_22_9_0_and_after(request):
print("this will not fire after in 22.9.0 and after")
app.register_middleware(this_fails_in_22_9_0_and_after, "request")This should work and will work. Or, rather, I cannot reproduce a scenario where it does not.
Adding to an app instance inside an
|
|
Thanks for the quick and thoughtful response! It's really the other issue, attaching middleware in I'm using Python 3.10 and running Sanic via poetry in Fedora Linux, but I originally noticed this problem in Docker on my Mac laptop. I haven't had a chance to try the minimal example below on my Mac today, but I will give it a try tomorrow and report back here. Can you see anything in my setup that would make this reproduce consistently for me? More details are below. My [tool.poetry]
name = "sanic-bug"
version = "0.1.0"
description = ""
authors = ["Semmy Purewal <redacted@redacted.com>"]
readme = "README.md"
[tool.poetry.dependencies]
python = "^3.10"
sanic = "22.6.2"
[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api" And my from sanic import Sanic, response
app = Sanic("my-hello-world-app")
@app.middleware("request")
async def this_always_works(request):
print("this is request middleware")
@app.before_server_start
async def before_server_start_listener(app, loop):
def this_fails_in_22_9_0_and_after(request):
print("this will not fire after in 22.9.0 and after")
app.register_middleware(this_fails_in_22_9_0_and_after, "request")
@app.route('/')
async def test(request):
return response.json({'hello': 'world'})
if __name__ == '__main__':
app.run(host="0.0.0.0", dev=True) Running Updating If you don't see anything obvious, we'll start digging into the code. |
|
I am not experiencing that |
|
Oh.... I see. 🤔 The difference was I had So the reason is that you are altering the router after it has tried to optimize itself. I will add something to the documentation about this. Add this: app.router.reset()
app.register_middleware(this_fails_in_22_9_0_and_after, "request")
app.finalize() |
|
@semmypurewal I am adding a convenience for this into the next release: @app.before_server_start
async def dynamic_router_changes(app: Sanic) -> None:
with app.amend():
app.add_route(...)
app.register_middleware(...) |
|
Couldn't that be handled internally within the router whenever any changes are done? Keep track of when it has already optimized and whether it needs to do that again, and/or postpone optimization until after server start? |
This could work for That is not a bad idea, but I still like the idea of wrapping this so that we an expose an API that allows us to continue to modify what happens inside and keep a constant external API. |
|
Wow, thanks for the quick turnaround on this! I can confirm that the same issue appears on my Mac. I also can confirm that your suggested code change resolves the issue. Can you clarify: is this a bug or just an inadvertent change? Or was this change expected? I'm wondering if this change should have been introduced as a major version update? I have 3 Sanic apps that I maintain, and this particular change affects two of the 3, meaning that they will no longer work if I update to 22.9+ without code changes. Does this mean all production apps that use this pattern will break on an update? In our case we have a few submodules that add middleware. From some quick testing, it looks like we'll have to update all of those callsites as well as the main app. I agree with @Tronic that it might be better to hide this from the user for two reasons: (1) so that existing apps that use this pattern will continue to work as expected (at least until a major version update), and (2) the proposed change seems to leak implementation details of the underlying framework (e.g. a user has to understand when these router optimizations happen, and then slightly change their code depending on whether it has already happened or not). In any case, thanks for your help! This gives us a workaround in the short term if we decide to upgrade. |
|
Hey @semmypurewal I want to jump in real quick and interject a) thanks for using Sanic! and b) a couple of notes First is that we do try our very best to include deprecation notice two release cycles prior to breaking changes. I'm coming in late and have not looked over the whole issue or dug in to it, so I do want to say that a non-LTS release may have breaking changes. We do our very best to limit this and when we know about it, advise the community that it's coming. With that being said, my suggestion may be to move to 22.12 LTS which, for production apps, is supported for 2 years (thru 2024) and means you should not have any breaking changes to worry about, and we will backport important fixes and security fixes. Here's the release policies, and please reach out on discord (if you haven't already) - happy to help. |
|
@sjsadowski Ah, got it! Thank you. I was thinking you were using semantic versioning but now I see you're not. We will look into migrating to an LTS. |
|
Thanks god, I thought I was the only one who had such a problem. from sanic import Sanic
from sanic_jwt import Initialize
from config import Config
from helpers import ChatGptBot, DBClient
from middlewares import RequestMiddleware, ResponseMiddleware
from views import server_bp, jwt_bp
app = Sanic('chat-gpt')
Initialize(app, **Config.JWT_CONFIG, **jwt_bp)
app.blueprint(server_bp)
@app.listener('before_server_start')
async def start_listener(_, __):
app.ctx.bot = ChatGptBot(Config.OPENAI_KEYS)
app.ctx.database = await DBClient(**Config.DB_CONFIG)
@app.middleware('request')
async def create_database_session_middleware(request):
db_session = request.app.ctx.database.maker()
request.ctx.db_session = db_session
@app.middleware('response')
async def close_database_session_middleware(request, _) -> None:
await request.ctx.db_session.close()
@app.listener('before_server_stop')
async def stop_listener(_, __):
await app.ctx.database.close()
app.ctx.bot.close()
if __name__ == '__main__':
app.run(**Config.SERVER_CONFIG)After listening to @ahopkins's explanation, I know why this happened. |
|
@miss85246 Oooo exciting First one of these I've seen so far. Interested to see how this turns out. |
@Tronic The PR does this. It uses the application and routers' states to determine what to do. It will happen all implicitly now. @app.before_server_start
async def setup(app: Sanic):
app.exception(ZeroDivisionError)(zero)
app.add_route(handler, "/foo")
app.on_request(on_request)
app.add_signal(signal, "http.lifecycle.request") |
Is there an existing issue for this?
Describe the bug
Prior to 22.9.0, you could register request middleware in functions and listeners. Since 22.9.0 this has stopped working. I'm unclear on whether this is by design or not, but I wasn't able to find anything in the docs that explains the discrepancy.
The last working version I tested was 22.6.2.
Code snippet
Expected Behavior
I expected all middleware to fire, but only the first one I set up fires since 22.9.0. I know middlware priority was introduced in 22.9, but I wouldn't expect that to have broken existing apps. I tried explicitly setting the priority and I still was not able to get the middleware to fire.
How do you run Sanic?
As a script (
app.runorSanic.serve)Operating System
Linux
Sanic Version
22.9.0
Additional context
Thanks for Sanic!
The text was updated successfully, but these errors were encountered: