Re: [RFC] Context Managers

On Wed, 5 Nov 2025 at 23:39, Bob Weinand <bobwei9@hotmail.com> wrote: > > Hey Larry, Tim, Seifeddine and Arnauld, > > On 4.11.2025 21:13:18, Larry Garfield wrote: > > Arnaud and I would like to present another RFC for consideration: Context Managers. > > > > https://wiki.php.net/rfc/context-managers > > > > You'll probably note that is very similar to the recent proposal from Tim and > > Seifeddine. Both proposals grew out of casual discussion several months ago; I don't believe > > either team was aware that the other was also actively working on such a proposal, so we now have > > two. C'est la vie. :-) > > > > Naturally, Arnaud and I feel that our approach is the better one. In particular, as > > Arnaud noted in an earlier reply, __destruct() is unreliable if timing matters. It also does not > > allow differentiating between a success or failure exit condition, which for many use cases is > > absolutely mandatory (as shown in the examples in the context manager RFC). > > > > The Context Manager proposal is a near direct port of Python's approach, which is > > generally very well thought-out. However, there are a few open questions as listed in the RFC that > > we are seeking feedback on. > > > > Discuss. :-) > > I've been looking at both RFCs and I don't think either RFC is good > enough yet. > > > As for this RFC: > > It makes it very easy to not call the exitContext() method when calling > enterContext() manually. The language (obviously) doesn't prevent > calling enterContext() - and that's a good thing. But also, it will not > enforce that exitContext() gets ever called (and it also cannot, > realistically). > > Thus, we have a big pitfall, wherein APIs may expect enterContext() and > exitContext() to be called in conjunction, but users don't - with > possibly non-trivial side-effects (locks not cleared, transactions not > committed etc.). Thus, to be safe, implementers of the interface will > also likely need the destructor to forward calls to exitContext() as > well. But it's an easy thing to forget - after all, the *intended* usage > of the API just works. Why would I think of that, as an implementer of > the interface, before someone complains? > > Ultimately you definitely will need the capability of calling > enterContext() and exitContext() manually too (i.e. restricting that is > not realistic either), as lifetimes do not necessarily cleanly nest - as > a trivial example, you might want to obtain access to a handle which is > behind a lock. You'll have to enter the context of the lock, enter the > context of the handle, and close the lock (because more things are > locked behind that lock, including the handle). ... But you don't > necessarily want the hold on the lock to outlive the inner handle. In > short: The proposed approach only allows nesting contexts, but not > interleaving them. > > > Further, calling with() twice on an object is quite bad in general. But > it might easily happen - you have a function which wants a transaction. > e.g. function writeSomeData(DatabaseTransaction $t) { with ($t) { > $t->query("..."); } }. A naive caller might think, DatabaseTransaction > implements ContextManager ... so let's wrap it: with($db->transaction() > as $t) { writeSomeData($t); }. But now you are nesting a transaction, > which may have unexpected side effects - and the code probably not > prepared to handle it. So, you have to add yet another safeguard into > your implementation: check whether enterContext() is only active once. > ... Or, maybe a caller assumes that $t = $db->transaction(); with ($t) { > $t->query("..."); } with ($t) { $t->query("..."); } is fine - but the > implementation is not equipped to handle multiple calls to enterContext().. > > > Additionally, I would expect implementers to want to provide methods, > which can be called while the context is active. However, it's not > impossible to call these methods without wrapping it into with() or > calling the enterContext() method explicitly. One more failure mode, > which needs handling. > Like for example, calling $t->query() on a transaction without starting it. > > > I don't like that design, which effectively forces you to put safety > checks for all but the simplest cases onto the ContextManager > implementation. > And it forces the user to recognize "this returned object > DatabaseTranscation actually implements ContextManager, thus I should > put it into with() and not immediately call methods on it". (A problem > which the use() proposal from Tim does not have by design.) > > > The choice of adding the exception to the exitContext() is interesting, > but also very opinionated: > > - It means, that the only way to abort, in non-exceptional cases, is to > throw yourself an exception. And put a try/catch around the with() {} > block. Or manually use enterContext() & exitContext() - with a fake "new > Exception" essentially. > - Maybe you want to hold a transaction, but just ensure that everything > gets executed together (i.e. atomicity), but not care about whether > everything actually went through (i.e. not force a rollback on > exception). You'll now have to catch the exception, store it to a > variable, use break and check for the exception after the with block. > Or, yes, manually using enterContext() and exitContext(). > > > It feels like with() is designed to be covering 70% of the use cases, > with a load of hidden pitfalls and advanced usage requiring manual > enterContext() and exitContext() calls. It's not a very good solution. > > > As to the destructors (and also in reply to that other email from > Arnauld talking about PDO::disconnect() etc.): > > It's already possible today to have live objects which are already > destructed. It's extremely common to have in shutdown code. It's > sometimes a pain, I agree. But it's an already known pain, and an > already handled pain in a lot of code. > If your object only knows "create" and "destruct", there's no way for > a > double enterContext() (nested or consecutive) situation to ever happen. > (Well, yes, you *could* theoretically manually call __destruct(), but > why would you ever do that?) > > > Last thing - proper API usage forces you to use that construct. > > > To the use() proposal from Tim: > > This proposal makes it very simple to inadvertently leak the use()'d > value. I don't think the current proposed form goes far enough. > > However we could decide to force-destruct an object (just like we do in > shutdown too). It's just one single flag for child methods to check as > well - the object is either destructed or not. We could also trivially > prohibit nested use() calls by throwing an AlreadyDestructedError when > an use()'d and inside destructed object crosses the use() boundary. > > The only disadvantage is that there's no information about thrown > exceptions. I.e. you cannot add a default behaviour of "on exception, > please do this", like rolling transactions back. But: > - Is it actually a big problem? Where is the specific disadvantage over > simply $db->transaction(function($t) { /* do stuff */ }); - where the > call of the passed Closure can be trivially wrapped in try/catch. > - If yes, can we e.g. add an interface ExceptionDestructed { public bool > $destructedDuringException; }? Which will set that property if the > property is still undefined - to true if the destructor gets triggered > inside ZEND_HANDLE_EXCEPTION. To false otherwise. And, if an user > desires to manually force success/failure handling, he may set > $object->destructedDuringException = true; himself as a very simple - > one-liner - escape hatch. > > > The use() proposal is not a bad one, but I feel like requiring the RC to > drop to zero first, misses a bit of potential to save users from mistakes.. > The other nice thing about use() is that it's optional. You don't have > to use it. You use it if you want some scoping, otherwise the scope is > simply the function scope. > > > To both proposals: > > It remains possible by default to call methods on the object, after > leaving the with or use block. So some checking on methods for a safe > API is probably still required. > > I don't think it's possible to solve that problem at all with the > ContextManager RFC, except manual checking by the implementer in every > single method. But it's possibly possible to solve it with the use() RFC > in orthogonal ways - like a #[ProhibitDestructed] attribute, which can > be added onto a class (making it apply to all methods) or a specific > method and causes method calls to throw an exception when the object is > destructed. > Which is possible to provide by the language, as the language knows > about whether objects are already destructed, unlike e.g. the > ContextManager, where it would be object state, which has to be > maintained by the user. > > > TL;DR: ContextManagers are a buttload of pitfalls. use() is probably > better, with much less inherent problems. And with the remaining > problems of the proposal being actually solvable. > > > Thanks, > > Bob > Hi Bob, I agree with your points, especially this one: > It makes it very easy to not call the exitContext() method when calling enterContext() > manually. [...] Thus, we have a big pitfall... This is the exact reason why, in the use() thread, I suggested a Disposable interface should **not** have an enter() method. The language should just guarantee a single dispose() method is called when the scope is exited (successfully or not). This completely avoids the "unbalanced call" pitfall. SA tools can warn/error when dispose() is called manually. API authors should be aware that the resource might receive multiple dispose() calls and handle this gracefully. ----- Regarding your other valid concerns (nested calls, using objects after the block, "leaking" the variable), I believe those are all solvable by how the use() construct evolves, which is why I prefer its foundation. You noted: > It remains possible by default to call methods on the object, after leaving the with or use > block. So some checking on methods for a safe API is probably still required. You're right, but the use() proposal has a clear path to solve this, which is far superior IMO to the manual checks required by the ContextManager design. As discussed in the use() thread, we could later introduce: 1. A **Resource** marker interface: This would tell the engine to handle the object specially (e.g., use weak references in backtraces to prevent accidental refcount inflation). 2. A **Disposable** interface: This would have the dispose() logic and could (and probably should) also be a Resource. Furthermore, a Disposable interface opens up the possibility of being supported *outside* the use() construct entirely. The engine could guarantee dispose() is called whenever the object goes out of *any* scope, just like a destructor but with crucial exception awareness: ```php function x() { $disposable = new SomeDisposable(); return; // $disposable->dispose(null) called } function y() { $disposable = new SomeDisposable(); throw new Error(); // $disposable->dispose($error) called } ``` This would make Disposable a truly powerful, general-purpose RAII mechanism, not just a feature tied to use(). With these (future) additions, the use() construct could be enhanced to **enforce a no-escape policy** specifically for Resources. If use($r = get_resource()) finishes and $r still has references, the engine could throw an error. This combination would *programmatically prevent* the "use after free" pitfall you described, rather than relying on manual checks inside every single method. All of those powerful safety checks can be added in the future. I don't see why we need to cram them into the initial use() RFC. Its current __destruct-based implementation is **already useful today** for APIs designed to leverage RAII (like the Psl lock example). It's normal that few APIs are designed this way now; the feature doesn't exist yet. Psl just happens to support it because of its Hack origins. This seems like a much safer and more extensible path.