Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compose middleware only once. #565

Closed

Conversation

alexdutton
Copy link
Contributor

Should be more efficient, and means that middleware factories can hook in signal handlers
for processing responses in a guaranteed order (once we have signals, of
course).

Should be more efficient, and means that middleware can hook in signal handlers
for processing responses in a guaranteed order (once we have signals, of
course).
@asvetlov
Copy link
Member

I don't follow how signals can help?

@alexdutton
Copy link
Contributor Author

Ah, sorry. I was thinking it would help people that want to connect response_prepare signals in the same order that they've arranged their middleware. You'd have something like:

@asyncio.coroutine
def middleware_factory(app, handler):
    app.on_response_prepare.append(do_something)
    @asyncio.coroutine
    def middleware(request):
         …
         return (yield from handler(request))
    return middleware

At the moment, middleware_factory is called once per request, which means it can't be used to do this kind of thing, leading to one having to plug in the signal handler separately, keeping the orders in sync if you move any of them around.

@asvetlov
Copy link
Member

People should subscribe for signal not in middleware but on application setup stage.

Something like library.setup(app) for both adding middlewares and subscribing for signals.
Appending new signal handler on every middleware request makes a mess (should I describe why?).

Is it makes a sense?

@asvetlov
Copy link
Member

Your PR looks like optimization (I don't know how it affects aiohttp performance).
But I want to make nested sub-application support sometimes.
App should pass request to sub-app on route match and sub-app will apply request according own signal handlers and middlewares.
I don't know how implement it right now but I have strong feeling the change makes sense.

@alexdutton
Copy link
Contributor Author

nested sub-applications, as in nesting routers? Sounds reasonable to me. I did play with a Host-based router at one point, but had trouble with making reverse lookups on routes work in an elegant way (i.e. request.app.router['routename'].url()).

Shall I profile the proposed change to see how useful it actually is, or perhaps look at the sub-app/route issue?

@asvetlov
Copy link
Member

That's up to you. Personally I curious about speed improvement after your change.
We can commit it now if value matters.

Subapp support is a next big rock after signals, requires non-trivial changes.

@asvetlov
Copy link
Member

Close as nested apps are incompatible with PR.
Feel free to reimplement it on top of current code but please make boost measure.

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants