Add SIGTERM handling in web.run_app

[cecton]

What do these changes do?

Makes web.run_app() handle SIGTERM by default to gracefully exit the application.

Are there changes in behavior for the user?

The application run with web.run_app() will override any existing SIGTERM handler in the current process.

Related issue number

#1932

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new entry to CHANGES.rst
  • Choose any open position to avoid merge conflicts with other PRs.
  • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

[cecton]

The testing is a bit complicated so I only tested using the script given in the original issue.

[codecov-io]

Codecov Report

Merging #1946 into master will decrease coverage by <.01%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1946      +/-   ##
==========================================
- Coverage   97.08%   97.07%   -0.01%     
==========================================
  Files          37       37              
  Lines        7613     7622       +9     
  Branches     1329     1330       +1     
==========================================
+ Hits         7391     7399       +8     
  Misses        100      100              
- Partials      122      123       +1

Impacted Files	Coverage Δ
aiohttp/web.py	99.65% <90%> (-0.35%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78e4bb8...2687965. Read the comment docs.

[asvetlov]

The test is still required.

I could suggest the following:

1. Create a script in temp folder which creates an application and registers on_cleanup signal.
   The signal code should do something visible to outer system like file creation.
2. In test code create a subprocess to execute the script.
3. Send SIGTERM to it
4. Wait for requested file existence.
5. Delete a script and tmp file on test cleanup

[asvetlov]

https://docs.python.org/3/library/asyncio-eventloop.html#unix-signals should be used for signal registration.
Also signal subscription should be skipped on Windows

[asvetlov]

maybe handle_sigterm is better name?

[cecton]

Yes actually I was thinking that you may want actually that run_app handles all the signals or none. For the user it's not really about handling SIGTERM in particular, it's more about handling the signals (INT and TERM) or not. It's unfortunate somehow that Python handles SIGINT for you by default.

I just realize now that maybe we should also handle SIGINT using https://docs.python.org/3/library/asyncio-eventloop.html#unix-signals but I'm not sure at all about it, what do you think?

[asvetlov]

SIGINT is handled out of the box, why should we care about it?

[cecton]

I actually answered later in the PR comments. It's not really "out of the box" for the event loop, it's out of the box for default synchronous usage of Python.

[cecton]

@asvetlov Do you mind if I move the print() inside the try...except? 87fa801

Otherwise I can't possibly know when to terminate.

[asvetlov]

Do you mind if I move the print() inside the try...except?
I totally ok with it

[asvetlov]

Could you use asyncio api for subprocesses in test instead of subprocess.Popen?
It's not critical but why not if it is exist.

[cecton]

@asvetlov Yes that's what I wanted in the beginning but the test method is not asynchronous. There is no point and it would make thing unreadable.

[cecton]

@asvetlov I mean:

The test function is synchronous so I suppose the tests are run synchronously. The thing I'm testing spawns a process, wait for it and assert. There is only one thing done at a time.

If I want to make it asynchronous it means I will have to move the test to a coroutine and the test itself will do a run_until_complete on that coroutine.

There is no point of doing so because the event loop will be busy with only one task. And the perk of using asyncio.create_subprocess is also not so good but it is actually using subprocess behind the scene (it's just a wrapper).

So unless you know how to make the tests run asynchronously in parallel like mocha.parallel, there is no real advantage to this.

async def real_test():
    proc = await asyncio.create_subprocess....
    async for line in proc.stdout:
        ....
    proc.terminate()
    retcode = await proc.wait()
    assert ...

def test_sigterm(loop):
    loop.run_until_complete(real_test())

[cecton]

@asvetlov I just checked the code of asyncio and I believe we should also manage the SIGINT the same way.

The reason is simply that if not managed, Python has this default behavior to raise KeyboardInterrupt. But apparently they made add_signal_handler in asyncio because using signal.signal wasn't enough.

This is from asyncio:

    def add_signal_handler(self, sig, callback, *args):
(...)
        handle = events.Handle(callback, args, self)
        self._signal_handlers[sig] = handle

        try:
            # Register a dummy signal handler to ask Python to write the signal
            # number in the wakup file descriptor. _process_self_data() will
            # read signal numbers from this file descriptor to handle signals.
            signal.signal(sig, _sighandler_noop)

            # Set SA_RESTART to limit EINTR occurrences.
            signal.siginterrupt(sig, False)
(...)

This is the doc for siginterrupt:

Change system call restart behaviour: if flag is False, system calls will be restarted when interrupted by signal signalnum, otherwise system calls will be interrupted. Returns nothing. Availability: Unix (see the man page siginterrupt(3) for further information).

Note that installing a signal handler with signal() will reset the restart behaviour to interruptible by implicitly calling siginterrupt() with a true flag value for the given signal.
From https://docs.python.org/3/library/signal.html#signal.siginterrupt

The other reason that makes me think we should handle it is that they are managing the signals with the event loop itself.

I'm not sure what are the consequences because KeyboardInterrupt and the signal.signal call I just added work fine in a simple example but I have a strong feeling about it. I propose to keep the variable handle_signal and actually handle INT and TERM with add_signal_handler.

[cecton]

@asvetlov I'm going to give up the signal testing on Windows if you don't mind...

Apparently the console ask to confirm when you send a signal CTRL_BREAK_EVENT or CTRL_C_EVENT:

tests\test_resolver.py ssssss....s.sss..
tests\test_run_app.py ...................sssss.s.^CTerminate batch job (Y/N)? 

By the way, you may have notice that your Windows CI (appveyor) is a bit stuck? This is because each build wait 1 hour before being killed. Since there are 6 builds every time, it's waiting 6 hours. I didn't realize fast enough and I'm afraid I'm blocking your CI for 7 hours now and there are still 5 hours to go.

Hum.... sorry?

[asvetlov]

Oooh. If Windows hangs with user prompt -- please don't run test on it. I'm fine with your proposal.
Don't worry about appveyor blocking -- I believe it was not intentional :)

[cecton]

@asvetlov ok finally done :) Are you sure about the subprocess? Did I convince you?

[asvetlov]

Thanks

[lock]

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.