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".
It can be reproduced by enabling the Perl debugger inside a PSGI application:
{
package DB;
sub DB { }
sub sub { &$sub }
}
$^P = 0x73f;
sub { [200, ['Content-Type' => 'text/plain'], ['Hello World']] }
For every request the following warnings are emitted:
Attempt to free unreferenced scalar: SV 0xfea6e8, Perl interpreter: 0xd534a0.
Attempt to free unreferenced scalar: SV 0xfea718, Perl interpreter: 0xd534a0.
where the unreferenced scalars are the uwsgi::input/uwsgi::error instances
created in build_psgi_env.
The calling convention for Perl subroutines is that the values pushed on the
stack must be mortalized in the callee, and if the caller wants to retain them,
it must do a SvREFCNT_inc to undo the effect of the mortalization.
Before this patch XS_input/XS_error were not mortalizing the value, and
uwsgi_perl_obj_new was not incrementing the reference count, so the two bugs
balanced each other.
When running under debugger, Perl forwards all function/method calls to
DB::sub, which causes a mortal copy of the return value of
uwsgi::input/error::new to be pushed on the stack. The value is cleared by the
FREETMPS at the end of uwsgi_perl_obj_new, and the freed value is added to the
environment hash. The warning is emitted at the end of the request when the
environment hash is freed and Perl notices that some of the values has been
already freed.
newRV(sv_newmortal()) is equivalent to newRV_noinc(newSV(0)): the former
creates a new SV with refcount 1, schedules a decrement "soon" (the
mortalization) and increments the refcount, the net result is a refcount of 1,
which is what the latter does.
This newly added support for read() offsets started causing "Use of
uninitialized value in subroutine entry" warnings.
This is all because there was a test for the number of items on the
stack, which ignored that the first argument is always the object, so 3
arguments to read() actually yields 4 arguments on the stack, not 3.
As a result we'd be calling SvIV() on a stack item that wasn't actually
passed in.
In 2.0.1-41-g3480c30 I introduced a regression with how the top-level
stackframe would appear within Perl programs. Before we'd show the
filename of the *.psgi file, but after we just showed "-e".
We can retain the bugfix I added in 2.0.1-41-g3480c30 while having a
sensible stacktrace by overriding the file via the #line directive.
The psgi loaded was calling perl_parse() with the script ostensibly to
set up xsinit.
However it would also call perl_parse() with the path to our *.psgi
file, whith the result that any BEGIN block in the *.psgi file would be
run twice, but anything outside BEGIN blocks would only run once.
This means that any code within explicit BEGIN blocks will run twice,
and any "use" statement in the *.psgi file will run its import() routine
twice, but due to the module being in %INC already we won't actually
compile things twice.
The previous behavior dates all the way back to the initial introduction
of the PSGI plugin in 299fd9c.
Then when support for local::lib was added in 7cbe751 we initially did a
perl_eval_pv() of a "use" statement like I'm doing here again now, but
later on in 1561dd3 changed it to call perl_parse with the commit
message "another PSGI loading fix".
Since there's no info on what that fixed or what was broken before I
have no idea if I'm introducing a regression here, but I don't see why
this way of loding local::lib shouldn't work, and it correctly munges
@INC for me when I try it.
We may still have this bug in the remaining perl_parse() calls that
remain for supporting "preinit" and "mule".
I haven't tested those modes (I don't use them), but when we load the
Perl apps we should only perl_parse() once with -e1, and then
perl_eval_pv() to actually load the application. We should not call
perl_parse() on code that we're just about to perl_eval_pv(), or we'll
run into this bug.
To test this just run:
./uwsgi --http 127.0.0.1:8080 --psgi ./t/perl/test.psgi
It'll no longer PANIC on the BEGIN block being run twice now, at least
in that simplistic non-"preinit" non-"mule" mode.
protoCall7