Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy->__destruct called although service's __construct() was never called #373

Closed
cjost1988 opened this issue Jun 9, 2017 · 11 comments
Closed

Comments

@cjost1988
Copy link

Hey,

as I can see there were some issues regarding the proxy->__destruct() reported before. I am running into the same issues like mentioned there.

An external service establishes connections on instance construction. Everything is fine until the proxy->__destruct() is invoked that itself forces instantiation of the service although it's not needed.

Is there any specific reason that you force instantiation of the proxied service on __destruct()?

In my point of view that means additional performance costs that are not wanted - just creating the instance reserves additional memory. With this behavior you force the developers to not load data or establish connections on construction if they want to avoid the addition performance overhead.

cheers,
Chris

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

Linking #258
Linking #262

Is there any specific reason that you force instantiation of the proxied service on __destruct()?

As already described in the issues above, this library cannot make any assumptions about the behavior of __destruct. Specifically, __destruct may be running critical parts of your logic, and therefore cannot be "skipped" or "ignored" by this library.

In my point of view that means additional performance costs that are not wanted - just creating the instance reserves additional memory. With this behavior you force the developers to not load data or establish connections on construction if they want to avoid the addition performance overhead.

I don't force anything - the library is agnostic, and it will try honoring the initial behavior as much as possible.

As a workaround, I suggest implementing an intermediate class that doesn't implement __destruct, and all it does is forwarding method calls: you can then proxy that one class just to remove __destruct from the signature, and therefore from the initialization at engine shutdown.

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

Just adding an example of how this would work, since this hackery would be EXTREMELY complex to add at ProxyManager level:

class MyRedis
{
    public function __construct(string $dsn) { /* connect to redis here */ }
    public function doSomething() { /* interact with redis here */ }
    public function __destruct() { /* disconnect here */ }
}

class MyRedisWithoutSillyConstruct extends MyRedis
{
    public function __construct(MyRedis $originalInstance) { $this->originalInstance = $originalInstance; }
    public function doSomething() { return $this->originalInstance->doSomething(); }
    // no constructor
}

$lazyService = $factory->createProxy(MyRedisWithoutSillyConstruct::class, ...);

@cjost1988
Copy link
Author

cjost1988 commented Jun 9, 2017

Yes, I understand your point. But __destruct() makes only sense when the instance was constructed before?

Am I totally wrong with the assumption that the proxy is instantiated as placeholder for the real service until the real service is requested and therefor instantiated?

So in a real world without the proxy mechanism the __destruct() would also not be called if I would never instantiate my real service.

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

But __destruct() makes only sense when the instance was constructed before?

Does it? A lot of stuff like session handling, profiling, etc uses __destruct to act at application shutdown.

Am I totally wrong with the assumption that the proxy is instantiated as placeholder for the real service until the real service is requested and therefor instantiated?

That's what every proxy implementing the LazyLoadingInterface does, yes. Still, when __destruct is implemented, your service/object will ALWAYS be used by the engine anyway: the PHP engine is the main consumer of that method, and it's always there.

So in a real world without the proxy mechanism the __destruct() would also not be called if I would never instantiate my real service.

In most cases, yes, you probably don't want initialization via __destruct() if the object was never used, but this is an assumption that can only be applied if you know exactly what the object is used for. This library can't imply that for you: it will not take any decisions about what to do with your object except for respecting its signature in the most faithful way. If you have a __destruct() defined on your object, ProxyManager will replicate it as-is without questioning why it is there: it is impossible for this library to know the "why".

@cjost1988
Copy link
Author

cjost1988 commented Jun 9, 2017

the PHP engine is the main consumer of that method

Yes it is. But only on an concrete instance. The destruct is invoked by PHP on every object which is not referenced anymore or at the script shutdown.

ProxyManager will replicate it as-is without questioning why it is there: it is impossible for this library to know the "why".

Yes, but the proxy pattern adds the logic that a target service will not be instantiated until it's used. So I can argument that the proxy library does exactly what you not want, execute behavior that would not be executed without the proxy.

Yes, I agree to you that the proxy needs to be equivalent to the target service but it needs to be equivalent in the way of usage as well, what means that if I don't instantiate the service then my __destruct() should never be called as well. That's what PHP would do.

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

Yes it is. But only on an concrete instance. The destruct is invoked by PHP on every object which is not referenced anymore or at the script shutdown.

A profiler may need to run and be initialized after all headers have been sent. Same for an error handler/collector. A perfectly valid example of this could be following:

class Profiler {
    public function __construct(PDO $db) { ... }
    public function profile($something) { ... }
    public function __destruct() {
        $this->db->insert($this->profileMetadata()); 
        $this->db->insert($this->getAccumulatedProfiles());
    }
}
$profiler = $proxyFactory->create(Profiler::class, function () {
    return new Profiler(connect_to_profiler_db();
});;
(new App($profiler))->run();

This will either initialize the profiler at runtime, when the application does something with it, or in any case at __destruct, after everything was already sent to the client. Session handlers do the same, and sometimes need to use __destruct to refresh the session identifier before it expires.

I can't agree that the above is good code (I don't think it is), but it is real-world code that works with this library (I've seen it done before too).

Yes, but the proxy pattern adds the logic that a target service will not be instantiated until it's used.

Your service is ALWAYS used at __destruct time, by the engine. The "why" is up to the author of that code. While __destruct is usually about clean-up operations, this is not an enforced limitation, and therefore can't be put in the assumptions of this library.

The workaround I proposed above (implementing a middle-man class), is basically telling PHP that __destruct() is not part of the public API of that class, but is rather an implementation detail. Having a public function __destruct() means that the proxy will also have it: no questions asked.

@cjost1988
Copy link
Author

cjost1988 commented Jun 9, 2017

Your service is ALWAYS used at __destruct time, by the engine

No. Only if I did new MyService() the engine would execute the __destruct().

class MyService
{
    public function __construct(){}
    function __destruct(){}
}

$myServiceProxy = $proxyFactory->create(MyService::class, function () {
    return new MyService();
});

(new App())->run();

This would force MyService::__destruct() (not by the engine) only because the $myServiceProxy was instantiated. Engine invokes $myServiceProxy->__destruct().

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

You did new MyService() though: it's just lazily initialized, but it does exist, as a proxy ;-)

@cjost1988
Copy link
Author

Ok, I think I got your point now.
There are two competing expectations.

But I unterstand why you say that it's necessary that the __destruct() at least invokes the __construct.
If you use a Proxy in your environment it has to be handled as it would be a concrete instance of the target and that implies the __destruct().

Thank you for the long discussion.

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2017

@cjost1988 no worries! Thanks for your understanding: I considered adding a flag to ProxyManager before, but the solution provided above is simpler and easier to understand too.

@cjost1988
Copy link
Author

@Ocramius We've also talked about the possibility to add an additional flag to enable skipping __destruct() for services which use the __destruct() correctly and using it only for object lifecycle management.

But for now, we write a custom adapter which handles the expected proxy behavior.

Have a nice day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants