How do you kill a server?

GrantEdwards commented 10 years ago

Killing the initial/parent process doesn't seem to do anything: that process goes away, but there are still worker processes hanging around accepting connections. How is something like a Unix "init" script supposed to shut down a daemon running secure-smtpd server?

GrantEdwards commented 10 years ago

The root problem is that the pool of worker processes are being created without the daemon flag.

That means that when the parent process is killed, the worker children don't get terminated and will continue to handle incoming connections.

I've fixed my local copy to set the daemon flag on the child processes.

An important implication of this is that after the initial call to asyncore() returns, the parent process needs to hang around (if it returns, the daemon children will get auto-magically terminated).

Something like this...


class SSLSMTPServer(secure_smtpd.SMTPServer):
    def __init__(self):
        pass

    def process_message(self, peer, mailfrom, mailto, message_data):
        print message_data

    def start(self):
        secure_smtpd.SMTPServer.__init__(
            self,
            ('0.0.0.0', 1025),
            None,
            require_authentication=True,
            ssl=True,
            ssl_version=ssl.PROTOCOL_SSLv3,
            credential_validator=MyCredentialValidator(),
            maximum_execution_time = 1.0,
            process_count = 1
        )
        asyncore.loop()

server = SSLSMTPServer()
server.start()

while 1:
   time.sleep(1.0)

By setting the daemon flag on the children and then putting the parent to sleep the server is now well behaved and can be killed by sending SIGTERM to the parent process.

bcoe commented 10 years ago

@GrantEdwards hey Grant, great find, do you want to try putting together a pull request to fix this?

GrantEdwards commented 10 years ago

I'll give it a shot. I don't use git very often (and I don't know if I've got it installed), so it may be a couple days before I get a chance to do it.

I was also thinking about working on a single-process option for users with no need to handle multiple simultaneous requests.

bcoe commented 10 years ago

Using something like libevent and greenlets would be an awesome hack, rather than subprocesses.

GrantEdwards commented 10 years ago

I've installed git, cloned the repository, and applied my fix to process_pool.py and to the example and benchmarking code. Before I submit a pull request, I\is there an autmated way to run tests/benchmarks?

GrantEdwards commented 10 years ago

I tried following the GitHub instructions for generating a pull request, but I don't have permission.

Is there a way to submit a patch?

bcoe commented 10 years ago

Hey Grant,

If you go to the secure-smtpd library, and click the "fork" button, it will create a copy of secure-smtpd on your account. You can then:

clone your copy of the project.
create a branch for your fix, git clone -b shutdown-fix
push to your branch.
and then submit a pull-request to my project.

Thanks for the hard work,

-- Ben,

On 13 May 2014 23:08, GrantEdwards notifications@github.com wrote:

I tried following the GitHub instructions for generating a pull request, but I don't have permission.

Is there a way to submit a patch?

— Reply to this email directly or view it on GitHubhttps://github.com/bcoe/secure-smtpd/issues/9#issuecomment-43019641 .

GrantEdwards commented 10 years ago

Ah, thanks. The pull request instructions I found on GitHub didn't mention the "fork" step.

I'll give a try over the week-end.

GrantEdwards commented 10 years ago

I've forked the project at created two branches in my copy: parent-worker and shutdown-fix. (I assume you can view diffs between your master and my branches.)

parent-worker

In parent-worker, multiprocessing is not used at all if the process_count is 1. The work is all done in the main process using asyncore the same way it is in the standard smtpd module. IIRC, this means that the server's start() method never returns. If process_count is > 1, it works the same as before (server.start() returns when the first connection comes in).

[The server app I wrote handles a few messages per week, so I'd probably just run with a single process.]

Using the above modification, I compared benchmark results with 1 process and with 5 worker processes. Throughput was the same (24-25 messages per second) in both cases, so it looks like I was doing something wrong or something else in the benchmark setup is throttling performance.

shutdown-fix

In shutdown-fix, I've set the daemon flag on the child processes so that they get killed if the main process terminates normally. If the main process terminates abnormally (SIGKILL, unhandled SIGTERM, etc.) the children are still left running and will continue to handle incoming SMTP connections (they will each need to be killed individually).

I've added code to the example programs so that after the server's start() method returns (which happens after the first request comes in) the main process installs a handler for SIGTERM and then goes into an idle loop. This seems a bit ugly, and I was wondering if the SIGTERM handler and idle loop code should be moved into the server's start() method. This would mean that the start() method never returns.

Questions

Are there applications that call server.start() and then do some useful work in the parent process after the first request comes in, the children start, and server.start() returns?
Should I put signal handling and idle loop code in the start() method so that the application doesn't have to do it?
Should I merge these two branches and submit a single pull-request, or would you prefer separate pull-requests? If the latter, both based on the same master version? or one on top of the other?

[It looks like I keep unintentionally adding a newline to the ends of files that didn't originally end with a newline -- I think emacs must be doing that in certain circumstances. I can undo those particular changes if you prefer that your files not end in newlines.]

bcoe commented 10 years ago

Hey Grant, I had a chance to look at this pull today. Things are looking good.

-

Are there applications that call server.start() and then do some useful work in the parent process after the first request comes in, the children start, and server.start() returns?

I don't think so, for the most part the parent process is just handling spawning processes and contains the async.core logic.

-

Should I put signal handling and idle loop code in the start() method so that the application doesn't have to do it?

I like this idea, I think that we should just give people signal handling by default; I'd then just test with the examples and make sure that everything runs well. In the future, it would be awesome for us to get some unit tests in place -- I was not a good open-source citizen when I wrote this a few years back :)

-

Should I merge these two branches and submit a single pull-request, or would you prefer separate pull-requests? If the latter, both based on the same master version? or one on top of the other?

I'd submit as two pull requests, if you have a chance to do so today, I'm working on side-projects all day :)

-- Ben.

On 17 May 2014 11:21, Grant Edwards notifications@github.com wrote:

I've forked the project at created two branches in my copy: parent-worker and shutdown-fix. (I assume you can view diffs between your master and my branches.) parent-worker

In parent-worker, multiprocessing is not used at all if the process_count is 1. The work is all done in the main process using asyncore the same way it is in the standard smtpd module. IIRC, this means that the server's start() method never returns. If process_count is > 1, it works the same as before (server.start() returns when the first connection comes in).

[The server app I wrote handles a few messages per week, so I'd probably just run with a single process.]

Using the above modification, I compared benchmark results with 1 process and with 5 worker processes. Throughput was the same (24-25 messages per second) in both cases, so it looks like I was doing something wrong or something else in the benchmark setup is throttling performance. shutdown-fix

In shutdown-fix, I've set the daemon flag on the child processes so that they get killed if the main process terminates normally. If the main process terminates abnormally (SIGKILL, unhandled SIGTERM, etc.) the children are still left running and will continue to handle incoming SMTP connections (they will each need to be killed individually).

I've added code to the example programs so that after the server's start() method returns (which happens after the first request comes in) the main process installs a handler for SIGTERM and then goes into an idle loop. This seems a bit ugly, and I was wondering if the SIGTERM handler and idle loop code should be moved into the server's start() method. This would mean that the start() method never returns. Questions

-

Are there applications that call server.start() and then do some useful work in the parent process after the first request comes in, the children start, and server.start() returns?

Should I put signal handling and idle loop code in the start() method so that the application doesn't have to do it?

Should I merge these two branches and submit a single pull-request, or would you prefer separate pull-requests? If the latter, both based on the same master version? or one on top of the other?

[It looks like I keep unintentionally adding a newline to the ends of files that didn't originally end with a newline -- I think emacs must be doing that in certain circumstances. I can undo those particular changes if you prefer that your files not end in newlines.]

— Reply to this email directly or view it on GitHubhttps://github.com/bcoe/secure-smtpd/issues/9#issuecomment-43417579 .

GrantEdwards commented 10 years ago

The change to include signal handling will result in changs to the example applications.

Would be OK if I also changed the examples so they used non-privledged port numbers?

That makes it much easier to test.

bcoe commented 10 years ago

That would be awesome :)

On 20 May 2014 10:48, Grant Edwards notifications@github.com wrote:

The change to include signal handling will result in changs to the example applications.

Would be OK if I also changed the examples so they used non-privledged port numbers?

That makes it much easier to test.

— Reply to this email directly or view it on GitHubhttps://github.com/bcoe/secure-smtpd/issues/9#issuecomment-43659952 .

bcoe / secure-smtpd