From 7ef33950c5fbf07eb1b8b9ac369a17adfd4ca424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 10 Nov 2014 18:53:10 +0000 Subject: [PATCH 1/4] psgi: Ensure that we call any DESTROY hooks on psgix.harakiri.commit Before this we'd just exit(0) and let the OS clean up after us, but e.g. with post-buffering=1 we'll end up with a temporary file in /tmp that we won't clean up when we exit unless DESTROY is called. This resulted in us leaking files in /tmp if we ever had a request where the last request before a harakiri was a POST request with a body we'd buffer to /tmp. We'd have similar leaks in any user-defined code that required DESTROY to run. Aside from this I'm still not very comfortable with what this whole code here in psgi_plugin.c and psgi_loader.c is doing when managing the interpreter(s). It: * Doesn't consistently call PERL_SET_CONTEXT() as described in "perldoc perlembed". * Nothing calls PERL_SYS_TERM() either. * Should we be calling uwsgi_perl_free_stashes() here too? To test this: UWSGI_PROFILE=psgi python uwsgiconfig.py --build ./uwsgi --master --http-socket localhost:1234 --psgi t/perl/test_harakiri.psgi Then elsewhere: curl 'localhost:1234?0' curl 'localhost:1234?1' Both of those should emit "Calling DESTROY". --- plugins/psgi/psgi_plugin.c | 17 +++++++++++++++++ t/perl/test_harakiri.psgi | 19 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 t/perl/test_harakiri.psgi diff --git a/plugins/psgi/psgi_plugin.c b/plugins/psgi/psgi_plugin.c index 337a2c33..ec1c66a7 100644 --- a/plugins/psgi/psgi_plugin.c +++ b/plugins/psgi/psgi_plugin.c @@ -710,7 +710,24 @@ void uwsgi_perl_after_request(struct wsgi_request *wsgi_req) { // async plagued could be defined in other areas... if (wsgi_req->async_plagued) { + int i; + uwsgi_log("*** psgix.harakiri.commit requested ***\n"); + + // clear the env, make sure any DESTROY attached to it will + // run. + SvREFCNT_dec(wsgi_req->async_environ); + + // We must free our perl context(s) so any DESTROY hooks + // etc. will run. + for(i=0;i{'psgix.harakiri'}; + + $env->{'psgix.harakiri.tester'} = bless {} => 'psgix::harakiri::tester'; + my $harakiri = $env->{QUERY_STRING}; + $env->{'psgix.harakiri.commit'} = $harakiri ? 1 : 0; + + return [200, [], [ $harakiri ? "We are about to destroy ourselves\n" : "We will live for another request\n" ]]; +} From 1dcdb720541c66fe9c359627f0b0433706ac11eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 11 Nov 2014 12:51:50 +0000 Subject: [PATCH 2/4] psgi: Move the destruction of the perl interpreter to the atexit hook + more bugfixes My previous commit ensured that we called the DESTROY hook but we wouldn't properly call the atexit hook. Now when you start uWSGI as instructed in that commit you'll get this: * localhost:1234?0: $$: Calling DESTROY * localhost:1234?1: *** psgix.harakiri.commit requested *** ...The work of process 523 is done. Seeya! 523: Calling the atexit hook 523: Calling DESTROY * Ctrl+C (or stop): ^CSIGINT/SIGQUIT received...killing workers... 696: Calling the atexit hook Before this change we wouldn't call the atexit hook when psgix.harakiri.commit was requested by calling localhost:1234?1. To make this work I removed the "if busy do not run atexit hooks" condition added in 1.4-rc2-246-g499202e. Of course uWSGI is going to think the worker is "busy", it's busy being destroyed, that doesn't mean we should skip running the atexit hooks. We'll still skip them under the "if hijacked do not run atexit hooks" condition added in that commit, and the "managing atexit in async mode is a real pain" condition added in 1.0.1-289-gd7e8523. Maybe those are further bugs that need to be solved, but I don't know how to test those modes. Now we'll also call PERL_SET_CONTEXT() and PERL_SYS_TERM() appropriately during destruction. Note that the latter should only be called once even if you have multiple interpreters. --- plugins/psgi/psgi_plugin.c | 55 +++++++++++++++++++++----------------- t/perl/test_harakiri.psgi | 6 +++++ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/plugins/psgi/psgi_plugin.c b/plugins/psgi/psgi_plugin.c index ec1c66a7..39a3f459 100644 --- a/plugins/psgi/psgi_plugin.c +++ b/plugins/psgi/psgi_plugin.c @@ -710,24 +710,10 @@ void uwsgi_perl_after_request(struct wsgi_request *wsgi_req) { // async plagued could be defined in other areas... if (wsgi_req->async_plagued) { - int i; - uwsgi_log("*** psgix.harakiri.commit requested ***\n"); - - // clear the env, make sure any DESTROY attached to it will - // run. - SvREFCNT_dec(wsgi_req->async_environ); - - // We must free our perl context(s) so any DESTROY hooks - // etc. will run. - for(i=0;i 0) - return; + goto destroyperl; realstuff: if (uperl.atexit) { uwsgi_perl_run_hook(uperl.atexit); } + +destroyperl: + + // We must free our perl context(s) so any DESTROY hooks + // etc. will run. + for(i=0;iasync_environ); + + // Destroy the PerlInterpreter, see "perldoc perlembed" + perl_destruct(uperl.main[i]); + perl_free(uperl.main[i]); + } + PERL_SYS_TERM(); + free(uperl.main); } static uint64_t uwsgi_perl_rpc(void *func, uint8_t argc, char **argv, uint16_t argvs[], char **buffer) { diff --git a/t/perl/test_harakiri.psgi b/t/perl/test_harakiri.psgi index 7680944a..35a40dbf 100644 --- a/t/perl/test_harakiri.psgi +++ b/t/perl/test_harakiri.psgi @@ -6,6 +6,12 @@ use warnings; sub DESTROY { print STDERR "$$: Calling DESTROY\n" } } +uwsgi::atexit( + sub { + print STDERR "$$: Calling the atexit hook\n"; + } +); + sub { my $env = shift; From 0a27a334cbbc1278ef6d51f201078c6a43c6799c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 11 Nov 2014 16:00:49 +0000 Subject: [PATCH 3/4] psgi: Remove the check for the async mode in uwsgi_perl_atexit According to Roberto this is just copy/pasted from the Python code, see 1.0.1-289-gd7e8523. --- plugins/psgi/psgi_plugin.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugins/psgi/psgi_plugin.c b/plugins/psgi/psgi_plugin.c index 39a3f459..b6dc29cd 100644 --- a/plugins/psgi/psgi_plugin.c +++ b/plugins/psgi/psgi_plugin.c @@ -863,10 +863,6 @@ static void uwsgi_perl_atexit() { if (uwsgi.workers[uwsgi.mywid].hijacked) goto destroyperl; - // managing atexit in async mode is a real pain...skip it for - // now -- TODO: explain why we skip it. - if (uwsgi.async > 0) - goto destroyperl; realstuff: if (uperl.atexit) { From e0e8bb1932b59993175404bb984ff867370dcdbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 11 Nov 2014 16:03:00 +0000 Subject: [PATCH 4/4] psgi: Don't free the wsgi_req->async_environ atexit, only in request context The atexit hooks can be called outside of the request context. See the discussion at https://github.com/unbit/uwsgi/pull/772 --- plugins/psgi/psgi_plugin.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/plugins/psgi/psgi_plugin.c b/plugins/psgi/psgi_plugin.c index b6dc29cd..93960531 100644 --- a/plugins/psgi/psgi_plugin.c +++ b/plugins/psgi/psgi_plugin.c @@ -708,6 +708,9 @@ void uwsgi_perl_after_request(struct wsgi_request *wsgi_req) { if (SvTRUE(*harakiri)) wsgi_req->async_plagued = 1; } + // Free the $env hash + SvREFCNT_dec(wsgi_req->async_environ); + // async plagued could be defined in other areas... if (wsgi_req->async_plagued) { uwsgi_log("*** psgix.harakiri.commit requested ***\n"); @@ -717,9 +720,6 @@ void uwsgi_perl_after_request(struct wsgi_request *wsgi_req) { goodbye_cruel_world(); } - // clear the env - SvREFCNT_dec(wsgi_req->async_environ); - // now we can check for changed files if (uperl.auto_reload) { time_t now = uwsgi_now(); @@ -854,7 +854,6 @@ void uwsgi_perl_run_hook(SV *hook) { static void uwsgi_perl_atexit() { int i; - struct wsgi_request *wsgi_req = current_wsgi_req(); if (uwsgi.mywid == 0) goto realstuff; @@ -876,10 +875,6 @@ destroyperl: for(i=0;iasync_environ); - // Destroy the PerlInterpreter, see "perldoc perlembed" perl_destruct(uperl.main[i]); perl_free(uperl.main[i]);