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

Response cleanup #2586

Open
ahopkins opened this issue Oct 24, 2022 · 7 comments
Open

Response cleanup #2586

ahopkins opened this issue Oct 24, 2022 · 7 comments
Milestone

Comments

@ahopkins
Copy link
Member

It would be nice to have a simple API to direct post-response cleanup efforts outside of the task. Consider the following example:

async def cleanup(request: Request, response: HTTPResponse):
    print("sleeping")
    await sleep(1)
    print("done")


@app.get("/")
async def handler(request: Request):
    response = await request.respond()
    await response.send("foobar")
    await response.eof()
    response.cleanup(cleanup)

In this example, we would immediately schedule a task using add_task API to allow the response to return as early as possible. But, it should also be smart enough to work even without eof() being called such that it would wait for the response to be sent and cleaned before proceeding to cleanup. Then you could even have handlers like this and guarantee that cleanup is run on a completed response.

async def cleanup(request: Request, response: HTTPResponse):
    print("sleeping")
    await sleep(1)
    print("done")


@app.get("/")
async def handler(request: Request):
    resp = json({"foo": "bar"})
    resp.cleanup(cleanup)
    return resp
@Tronic
Copy link
Member

Tronic commented Oct 24, 2022

I don't think there should be a callback API on response but rather something that you could do inline, e.g.

@app.get("/")
async def handler(request: Request):
    response = await request.respond()
    await response.send("foobar")
    await response.eof()  # trigger sending of response and enable pipelining to accept another request here
    await cleanup()

Perhaps with function other than .eof() to not affect existing code, if it currently doesn't run middleware etc. there (I don't remember what .eof() specifically does).

There would be much use for such functionality, including that of writing something to database or performing other operations that may take time after the response has already been sent, in many cases specific to one handler only and possibly requiring access to resources (such as async with blocks being open) within that handler, thus making a callback not a suitable solution where inline code or a normal function call are more flexible and natural.

@ahopkins
Copy link
Member Author

This was the original thought and is arguably a nicer api for all the reasons you mentioned. But it would be a huge breaking change since we allow the task to be Cancelled early when the client disconnects (which is what it would do with an explicit eof()--it sends the final bytes so the client knows it can close).

@ahopkins
Copy link
Member Author

What about making it a setter style decorator?

@handler.cleanup
async def handler_cleanup(...):

@Tronic
Copy link
Member

Tronic commented Oct 24, 2022

Still cannot apply within with or other control blocks nor be written as inline code without a separate function, variables need to be passed via request.ctx, and needs "custom syntax".

I guess that implementing it the way I suggested could be somewhat more complicated for Sanic's own implementation, but it also would make a lot more sense from app perspective. Other async frameworks such as NodeJS Express already function like this, and do not care when the handler returns.

Going for a decorator that way also means that the module where the cleanup is (possibly some utility module) needs to import the handler. The other way around (using cleanup decorator on the handler function) might be better and it preserves the same import direction as your original proposition. However, defining it inside the function as originally proposed at least allows passing variables as arguments, and having conditional execution if set in if statement.

The complication in implementing it the way I proposed would come that either the resumption of the pipeline or the remainder of the handler would need to be executed as a separate task, both of which are troublesome to implement, at least. Granted, Express gets this easier, with Javascript not having to await for function calls to return but can simply abandon them (as global tasks, if you will).

@ahopkins
Copy link
Member Author

Before posting this, I spent about 5 minutes looking to see what it would take to defer task cancellation when a client closes. I had abandoned the thought because it was certainly much more complex. But, potentially we can differentiate when you complete the response inside the handler and when it happens outside (ie, return HTTPResponse). Or, perhaps we differentiate early termination (when a client bails before receiving all bytes) and when termination happens after eof. In the latter, perhaps there is no such thing as a task cancellation.

@Tronic
Copy link
Member

Tronic commented Oct 27, 2022

I have no take on that. Only that sending the response inside handler was supposed to be first class API, and after-handler handling only a fallback that calls it (even if in practice most handlers would return a response, not send it).

@ahopkins
Copy link
Member Author

You are correct. Scrap the shortcut and let's fix this for real.

@app.get("/")
async def handler(request: Request):
    response = await request.respond()
    await response.send("foobar")
    await response.eof()

    # Client is free to close connection or start another request
    await do_something_here()
┌───────────────────┐    ┌────────────────┐     ┌───────────┐     ┌──────────┐
│ Connection closed ├───►│ Is in handler? ├──Y─►│ Eof sent? ├──Y─►│ Complete │
└───────────────────┘    └───────┬────────┘     └───┬───────┘     └──────────┘
                                 │                  │
                                 No                 N
                                 │                  │
                                 ▼                  │
                              ┌──────┐              │
                              │Cancel│◄─────────────┘
                              └──────┘

@ahopkins ahopkins added this to the 23.3 milestone Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants