@Scope("Singleton") ignored when using DiCompiler?

[ray-di]

@Scope("Singleton") ignored when using DiCompiler?

[koriym]

Hmm, It seems wrong behavior.

[craigjbass]

More investigation

It seems like Scope isn't handled, and all objects are treated as if they are prototypical?

Would the "logger" or the DependencyFactory be responsible for maintaining singleton references?

[koriym]

I add di compiler singleton tests. I'm adraid no error produced. Feel free to add your test.

[koriym]

logger or DependencyFactory doesn't handle singleton.
Injector care for it, logger log that activity. DependencyFactory re-create as logger logged.

I assumed that if we had a problem of singleton that is a problem of injector. This may wrong. I need test error case for more investigation.

[craigjbass]

Sorry I haven't forgotten about this - been busy. Will followup soon

[craigjbass]

There seem to be a few issues here:

1

CompilationLogger::getNotInjectedClass is called

but $this->objectStorage is null... this is because unserialize does not create this.

2

When building the cache in the first place, the code instantiates objects which could be dodgy for the rest of the request. Can the reflection occur without instantiating these objects?

E.g. if a constructor sets a constant, this cannot be undone for the rest of the request.

[koriym]

Hi @craigjbass
Welcome to back !

but $this->objectStorage is null... this is because unserialize does not create this.

Will followup soon...

[koriym]

1. CompilationLogger::getNotInjectedClass is calledbut $this->objectStorage is null.

This is not OK. but if getNotInjectedClass called, it is a bug or you are using illegal binding for Di compiler (like ->toInstance(new Foo) )

2. ... if a constructor sets a constant,

DiComipiler does not cache object. It creates instance each requrest.

[craigjbass]

I think the issue is that dependency factory never gets $this->instance set during compile time when the injector creates an instance and calls $this->logger->log( ... ). So consequently when the DiCompiler comes along to try to build the instance, the dependency factory will instantiate yet another instance, as it does not know about the reference already created..

What's more in my tests Injector::getInstance local variable $isSingleton doesn't appear to be trustworthy? I'm not sure why... my class @Scope("Singleton") annotation works perfectly when not using the DiCompiler.

[craigjbass]

On a side-note: The check if($this->instance) is naive as if an object has a method __toString that returns a falsey value it will skip over.
I suggest using if($this->instance !== null)

in DependencyFactory::get

[koriym]

@craigjbass thanks considering this issue.

$isSingleton should be ok if it wokrs perfect without DiCompiler. DiCompiler does not use $isSingleton.

I think the problem is we still don't have a test case for this issue.

[craigjbass]

Hopefully aa5a6bf makes sense

[koriym]

@craigjbass. Your PR and debug step execution clarify the problem. Thanks.

The problem is "instance container space". A native injector and DI compiler have separate instance container space. If we do "compile on demand", the both space is mixed. then cause the problem.(especially singleton)

But if we "reload", the problem is gone if instance container is successfully made for DI compiler and it is not mixed because DI compiler holds all object graph information.

I also found $isSingleton is not used and not showing correct status. This should be refactored but its not main reason for this issue. Thanks again this PR helps a lot to clarify the issue.

[craigjbass]

I think the DiCompiler could use the result from the Injector when it compiles a class

The second test case is the most important, as Singleton's usually are defined as particularly volatile in legacy code-bases.

The first test is merely a performance improvement, whereby a prototype does not need to be double-instantiated.

[koriym]

Yes, I thought same thing but seems we need to consider more.

Let's suppose if we use injector for model factory, A first factory creates object "model A" with compiler because already know how to make. But "model B" appeared after "A" is instantiated with compiler, but need to compile. The singleton dependency of "A" and "B" is not same. (But It suppose to be same in next request.)

[craigjbass]

this is where both the compiler and injector need to use aura as a container for existing singletons, if I'm correct in the way aura works.

I think the DependencyFactory may need to depend on more details. :(

[koriym]

Yes, but if we need a both in same time. DiCompiler originally designed to compile all class in compile time to get object graph information. Then I add "compile on demand" for the convenience.

[craigjbass]

Compile on demand is a good feature and is definitely in the spirit of PHP and the original injector. It would be good if they can be swapped out interchangeably as per the interface spec.

I will see if can find a simple solution tomorrow. I think it's possible.

[koriym]

Yes I agreed.

Let me tell some clarification for further idea.

• Injector knows which instance is singleton, then store singleton instance to "Aura Container"
• Compiler depends the dependency by number. (DependencyFactory's id). if object 1 and object 2 depends object 3, we can call object 3 is singleton but there is no special treatment.

[craigjbass]

The idea behind this commit is that ChildInjector can receive an object from DiCompiler, should it need to when fetching a dependency. DiCompiler will in turn call ChildInjector::getInstance if it doesn't know of that object.

I'm not sure on how DiCompiler::recompile works? Does it rebuild the dependencies every time?

[craigjbass]

I have fixed the behaviour in the case of Singletons... however I still feel that the DiCompiler is constructing an object one too many times... I'm still not sure of the foreach loop in DiCompiler::recompile as this seems to make no sense when compiling just one class? We should be now be able to use result from $this->injector->getInstance($class);? As $this->injector is a ChildInjector

[craigjbass]

b63d1ee was unsuccessful, it creates a lot of errors.

My reasoning behind how the compiler works is obviously not quite the real scenario.

[koriym]

I'm still not sure of the foreach loop in DiCompiler::recompile as this seems to make no sense when compiling just one class? We should be now be able to use result from $this->injector->getInstance($class);?

DiCompiler identify object by using object hash, which is changed every request. We can't add new graph to saved object graph. This is the reason "whole possibly appearing class" are re-compiled when new class is "discovered".

[koriym]

For example, object A, B, C use same annotation reader object. Saved object graph as they are, Then D is appeared but no way to identify same dependency if cached graph used.

DiCompiler compile A,B,C and D at once to draw correct object graph.

[mackstar]

I think I might also be experiencing this issue

$this
            ->bind('Symfony\Component\HttpFoundation\Session\Session')
            ->toProvider('Mackstar\Spout\Provide\Session\SessionProvider')
            ->in(Scope::SINGLETON);

In a module is resulting in 'Failed to start the session: already started by PHP. when the dependency is injected more than once in a request. This shows that it is being instantiated twice.

[koriym]

@craigjbass @mackstar I think I could finally fix this issue. (with big internal changes.)

[mackstar]

Wow! お疲れさまです〜！

[koriym]

Hi @craigjbass @mackstar

I'm thinking to dismiss @Scope annotation on next version of Ray.Di.

• considered bad design for some people.
• for things more simple. (one rule. one method to do one thing)

Any thought ?

[craigjbass]

Hey @koriym, we now use a in-house made dependency-injector. A lot of the ideas from the project helped architect that system, hopefully it will be released open-sourced soon.

With regards to removing the Singleton scope - I think this is a bad idea... It is useful to have Singleton and it is not an "anti-pattern" when used with DI.

The specific anti-pattern lurking within the Singleton pattern is hiding a global scope variable with public static methods:

class DodgySingleton {
   public static $instance; //dodgy global variable!

   public static function getInstance() {
        if( ! self::$instance instanceof DodgySingleton ) {
             self::$instance = new self();
        }
        return self::$instance;
   }

}

Obviously this doesn't exist with DI.

With regards to the implementation - I have implemented Scopes as their own "Scope" strategies

• \Clearbooks\Framework\Injector\Scope\Prototype, which just invokes creating a new Instance each time
• \Clearbooks\Framework\Injector\Scope\Singleton, which extends prototype and caches the Instance object
• \Clearbooks\Framework\Injector\Scope\Provider, which invokes a configured object with a special interface, but then wraps the received object in an Instance

These Scope objects are what get stored in the ScopeContainer, for each concrete resolved classnames. The returned Instance objects handle what methods need calling/have been called

[koriym]

hi @craigjbass nice to hear from you again.

What I mention about removing @Scope here are only for define by annotation.
DSL definition stays same as follows.

$this->bind(FooInterface::class)->to(Foo::class)->in(Scope::Singleton);

It is aim to remove knowledge of how to create from target. How is it ? You prefer annotate scope more than binding DSL ? That was the my question. Sorry for insufficient explanation.

The next version of Ray.Di is almost ready. 1) Compatibility 2) cleaner and smaller (less than half) full rewriten code (it marked 10.0 at scrutinizer ci) and 3) Performance (combined with compiler). Functionality comes later, Quality first.

It's interesting to see your project :) Keep good work !

[craigjbass]

It is aim to remove knowledge of how to create from target.
How is it ?

Yeah that makes sense... my solution is not annotations, but is not the most eloquent.
Can definitely appreciate that removing the tainting of application code with DI meta-data may not be ideal... However it does allow developers to quickly see the implementation...

assumes prototype and constructor injection only

class SomeClassA  {
   ...

}

reads static methods for meta-data

class SomeClass implementation \Clearbooks\Framework\Injector\Injectable {
   public static function getMethodsToInject() { return [ 'setSomeDependency' ]; }
   public static function getScope() { return self::SCOPE_SINGLETON }

   ...

}

class ProjectSettings implements IBindingDefiner {

   public function getBindings()
   {
      return [
          ( new Bind( IDatabaseAdapter::class ) )->to( MySqlDatabaseAdapter::class ),
          ( new Bind( IUserLocale::class ) )->toProvider( UserLocaleProvider::class ),

      ]

   }

}

You prefer annotate scope more than binding DSL ?

Yes

[koriym]

I see. You provide define class (meta data provider) instead of annotation.

[craigjbass]

Yeah - it's kind of okay. I don't like coupling implementation to the DI framework... So I can understand the reason for dropping annotations.

[koriym]

Is Annotation coupling ? I know if we put the topic aside, Still some people don't like use it. Or more importantly, We need inject non-annotated class sometime.

My answer (experimental implementation) is are follows on version 2.

protected function configure()
{
    $this
        ->bind(FakeCarInterface::class)
        ->toExplicit(
            FakeCar::class,
            (new InjectionPoints)->addMethod('setTires')->addMethod('setHardtop'),
            'postConstruct'
        )
       ->in(Scope::SINGLETON);
}

Maybe more syntax sugar needed. But the concept stay same.