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

A "while_serving" decorator? #13

Closed
jonozzz opened this issue Jan 12, 2019 · 17 comments
Closed

A "while_serving" decorator? #13

jonozzz opened this issue Jan 12, 2019 · 17 comments

Comments

@jonozzz
Copy link

jonozzz commented Jan 12, 2019

What do you think about having another decorator like this that would allow you to control when server is shut down

This is useful when you need a server up for to X seconds, rather than forever. I think you could spin up a thread with a timer and have that call loop.stop() when done, but it's dirty.

@app.while_serving
async def shutdown_when_idle_for_seconds():
    while True:
        if condition:
            raise Shutdown
        await sleep(1)

Great for testing!

@pgjones
Copy link
Owner

pgjones commented Jan 12, 2019

Not sure what the use case is here, why would you want the server to die completely? (Wouldn't you want it to close idle connections but listen for new ones).

@jonozzz
Copy link
Author

jonozzz commented Jan 12, 2019

I need to programmatically spin up a sever and a client to test an external load balancer. Having a server that runs forever is not really an option.

@pgjones
Copy link
Owner

pgjones commented Jan 12, 2019

Could you do something like this?

def shutdown():
    raise Shutdown()  # Or maybe sys.exit?

loop = asyncio.get_event_loop()
loop.call_later(1, shutdown)
loop.run_until_complete(serve(app, config))

I'm not sure this is a common enough use case to be part of Hypercorn.

@jonozzz
Copy link
Author

jonozzz commented Jan 13, 2019

One issue with that might be that "serve" would want to call run_forever at some point which is not possible, since the loop is already running.

I don't think it'd be too complicated to add, I see it as an addition to the "before_serving" and "after_serving" decorators.

@pgjones
Copy link
Owner

pgjones commented Jan 13, 2019

In this case serve is a coroutine, 1ecfcc3 so the run_until_complete shouldn't be an issue.

I see it as an addition to the "before_serving" and "after_serving" decorators.

So when would it be called? Some options I can think of are, after the first request, after every request, periodically (what period).

@jonozzz
Copy link
Author

jonozzz commented Jan 14, 2019

Is this serve() in the latest hypercorn version only compatible with quart 0.7.x? I can't make it work with quart 0.6 version on python 3.6.

@pgjones
Copy link
Owner

pgjones commented Jan 14, 2019

It should work with 0.6 on Python 3.6, what is the error you are getting? (I've not released it yet though).

@jonozzz
Copy link
Author

jonozzz commented Jan 14, 2019

Getting something like this

  File "http2_server.py", line 203, in run
    loop.run_until_complete(serve(app, config))
  File "/usr/lib/python3.6/asyncio/base_events.py", line 473, in run_until_complete
    return future.result()
  File "xyz/tmp/hypercorn/hypercorn/asyncio/__init__.py", line 31, in serve
    await worker_serve(app, config)
  File "xyz/tmp/hypercorn/hypercorn/asyncio/run.py", line 133, in worker_serve
    await lifespan.wait_for_startup()
  File "xyz/tmp/hypercorn/hypercorn/asyncio/lifespan.py", line 47, in wait_for_startup
    await asyncio.wait_for(self.startup.wait(), timeout=self.config.startup_timeout)
  File "/usr/lib/python3.6/asyncio/tasks.py", line 362, in wait_for
    raise futures.TimeoutError()
concurrent.futures._base.TimeoutError

Runnning on:

Python 3.6.5
Quart                     0.6.11     
Hypercorn                 0.4.6      (from master)

@jonozzz
Copy link
Author

jonozzz commented Jan 14, 2019

Here's an update... Enabled some error logging and got:
ASGI Framework Lifespan error, continuing without Lifespan support

Quart 0.6 doesn't seem to have the "lifespan" scope type.

@pgjones
Copy link
Owner

pgjones commented Jan 15, 2019

I've just tested this and it works ok for me (the ASGI Framework Lifespan error, continuing without Lifespan support is a warning that is safe to ignore with Quart 0.6). I think you likely have a before_serving function that doesn't complete e.g. the first snippet in this thread. Could you paste your before_serving function(s)?

@jonozzz
Copy link
Author

jonozzz commented Jan 15, 2019

I removed any decorators I had:

    config = Config()
    config.error_log_target = '-'
    config.certfile = certfile
    config.keyfile = keyfile
    config.bind = ["127.0.0.1:8002"]

    loop = asyncio.get_event_loop()
    loop.run_until_complete(serve(app, config))

How did you test it and how do you know it's working? The timeout exception comes after 1-2 minutes and until then any requests coming in are being refused.

@pgjones
Copy link
Owner

pgjones commented Jan 15, 2019

I see, it was a race condition - I've fixed it in 16b0ae8 could you see if it works for you?

@jonozzz
Copy link
Author

jonozzz commented Jan 15, 2019

Works now!
Back to my initial ask, I poked around the code a bit and noticed that you have a shutdown_event argument that I could use, but it doesn't bubble up to serve(). Any reason why it doesn't?
https://github.com/pgjones/hypercorn/blob/master/hypercorn/asyncio/run.py#L128

Also, would it make sense to have a similar startup_event or something like that, that's set when the server is ready to take new connections?

@pgjones
Copy link
Owner

pgjones commented Jan 21, 2019

I think your use case might be too specific for Hypercorn to support directly, I think the before/after serving functionality should cover almost all use. I'm actually not sure how it doesn't cover your case (I think it should).

@jonozzz
Copy link
Author

jonozzz commented Jan 23, 2019

But would it make sense to expose shutdown_event kwarg to serve coro?

@pgjones
Copy link
Owner

pgjones commented Jan 24, 2019

I don't think so as I'd like to keep the public API as simple as possible. You could use the worker_serve coro though (basically serve with the shutdown_event).

@pgjones
Copy link
Owner

pgjones commented Jan 31, 2019

Closing for now

@pgjones pgjones closed this as completed Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants