Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:107509
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
In-Reply-To: <CAF+90c_iPDv4RCCG-1vd=cVo-j8GO-BBf=m2KqBQm24xVFwNmg@mail.gmail.com>
Date: Fri, 11 Oct 2019 09:27:31 -0500
Cc: Marcio Almada <marcio.web2@gmail.com>,
 PHP internals <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <2406F2E7-CD65-4E30-A98C-C09FB52B4F2B@trowski.com>
References: <CAF+90c9EQ+UGe-y+V_n1QrYkXtPh1uWKXXWEJQUe0xP-MjV7_w@mail.gmail.com>
 <CAOsHV+tpnh9RvbBqXNjs2yp0muu_GHRf3CXVSxg9v081EDGRWQ@mail.gmail.com>
 <CAF+90c_iPDv4RCCG-1vd=cVo-j8GO-BBf=m2KqBQm24xVFwNmg@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Subject: Re: [PHP-DEV] exit() via exception
From: aaron@trowski.com (Aaron Piotrowski)



> On Oct 11, 2019, at 9:11 AM, Nikita Popov <nikita.ppv@gmail.com> =
wrote:
>=20
> On Fri, Oct 11, 2019 at 3:47 PM Marcio Almada <marcio.web2@gmail.com> =
wrote:
>=20
>> Em sex, 11 de out de 2019 =C3=A0s 08:05, Nikita Popov
>> <nikita.ppv@gmail.com> escreveu:
>>>=20
>>> Hi,
>>>=20
>>=20
>> Hello :)
>>=20
>>> Currently exit() is implemented using bailout and unclean shutdown, =
which
>>> means that we're going to perform a longjmp back to the top-level =
scope
>> and
>>> let the memory manager clean up all the memory it knows about. =
Anything
>> not
>>> allocated using ZMM is going to leak persistently.
>>>=20
>>> For me, one of the most annoying things about this is that we can't
>> perform
>>> proper leak checks on code using PhpUnit, because it will always =
exit()
>> at
>>> the end, which will result in "expected" memory leaks.
>>>=20
>>> I think it would be good to switch exit() to work by throwing a =
magic
>>> exception, similar to what Python does. This would allow us to =
properly
>>> unwind the stack, executing finally blocks (which are currently =
skipped)
>>> and perform a clean engine shutdown.
>>>=20
>>> Depending on the implementation, we could also allow code to =
actually
>> catch
>>> this exception, which may be useful for testing scenarios, as well =
as
>>> long-running daemons.
>>>=20
>>> I'm mainly wondering how exactly we'd go about integrating this in =
the
>>> existing exception hierarchy.
>>=20
>>> Assuming that it is desirable to allow people
>>> to actually catch this exception
>>> my first thought would be along these
>>> lines:
>>>=20
>>> Throwable (convert to abstract class)
>>> \-> Exception
>>> \-> Error
>>> \-> ExitThrowable
>>>=20
>>> This does mean though that existing code using catch(Throwable) is =
going
>> to
>>> catch exit()s as well. This can be avoided by introducing *yet =
another*
>>> super-class/interface above Throwable, which is something I'd rather
>> avoid.
>>>=20
>>=20
>> Since you brought python as inspiration, I believe the hierarchy goes
>> like this on their land:
>>=20
>> BaseException
>> +-- SystemExit
>> +-- KeyboardInterrupt
>> +-- GeneratorExit
>> +-- Exception
>>     +-- [kitchen sink]
>>=20
>> Being `BaseException` the base class for all built-in exceptions. It
>> is not meant to be directly
>> inherited by user-defined classes. It 's the equivalent to our
>> `Throwable` situation. In this context
>> `ExitThrowable -> Throwable ` appears legit.
>>=20
>>>=20
>>> Anyone have thoughts on this matter?
>>>=20
>>=20
>> Yes. There is an obvious can of worms if I've got this right: =
`exit()`
>> and `die()` would no longer guarantee a
>> program to actually terminate in case catching `ExitThrowable` is
>> allowed. Python solves this by actually
>> having two patterns:
>>=20
>> 1. `quit()`, `exit()`, `sys.exit()` are the equivalent to `raise
>> SystemExit`, can be caught / interrupted
>> 2. `os._exit()`, can't be caught but has a callback mechanism like =
our
>> `register_shutdown_function`,
>> see https://docs.python.org/3/library/atexit.html
>=20
>=20
> I don't believe atexit applies to os._exit(). In any case, I agree =
that
> this is something we're currently missing -- we should probably add a
> pcntl_exit() for this purpose. It should be noted though that this is
> really very different from exit(), which is still quite graceful and =
usable
> in a webserver context, while a hypothetical pcntl_exit() would bring =
down
> the server process. As the Python docs mention, the primary use-case =
would
> be exiting from forked processes without going through shutdown, which =
has
> also recently come up in https://github.com/php/php-src/pull/4712.
>=20
>=20
>> If we bind `exit()` and `die()` to a catchable exception how would we
>> still have the scenario 2 available
>> on PHP land without a BCB? :)
>>=20
>=20
>> I have one simple suggestion: Introduce `EngineShutdown -> =
Throwable`,
>> bind `exit|die` to it but disallow
>> `catch(\EngineShutdown $e)` at compile time. This would allow keeping
>> backwards compatibility to
>> scenario 2 without messing with our current exception hierarchy.
>>=20
>=20
> I think the options are basically:
>=20
> 1. Making EngineShutdown implement Throwable, which would make =
existing
> catch(Throwable) catch it -- probably a no-go.
>=20
> 2. Making EngineShutdown not implement Throwable, which means that not =
all
> "exceptions" implement the interface, which is rather odd. It still =
allows
> explicitly catching the exit.
>=20
> 3. Introducing a function like catch_exit(function() { ... }). This =
would
> still allow catching exits (for phpunit + daemon use cases), but the =
fact
> that this is actually implemented based on an exception would be =
hidden and
> the only way to catch the exit is through this function.
>=20
> 4. Don't allow catching exits at all. In this case the exception is =
just an
> implementation detail.
>=20
> Nikita

+1 for option 3.

EngineShutdown could be a special exception to the engine, being handled =
like an exception internally, but not implement Throwable and therefore =
not an exception from user-land's point-of-view.

EngineShutdown could be added to the list of "throwables", but forbid =
instigation in user-land.
=
https://github.com/php/php-src/blob/db233501ff9d56765ef4a870b777a643c21367=
11/Zend/zend_exceptions.c#L909-L916

No catch block would catch it, because it wouldn't implement Throwable =
nor extend Exception or Error.

Aaron Piotrowski