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

Manual app reset #4928

Merged
merged 4 commits into from Apr 17, 2018
Merged

Manual app reset #4928

merged 4 commits into from Apr 17, 2018

Conversation

@SamMousa
Copy link
Collaborator

@SamMousa SamMousa commented Apr 9, 2018

Some modules keep references to the Yii2 Client object.
I wrongly assumed that the Yii2 Module was the only one, and thus I did application cleanup in upon client destruction.
Application destruction is now done in _after().
An issue was fixed with the $pdoCache never getting filled.

SamMousa added 2 commits Apr 6, 2018
…good place for clean up. Instead manually reset application in _after
@samdark
Copy link
Collaborator

@samdark samdark commented Apr 9, 2018

Test seems to fail...

@samdark samdark added Yii BUG labels Apr 9, 2018
@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 9, 2018

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 9, 2018

@samdark as far as I can tell tests fail due to a PHPUnit upgrade from last Friday.
Issues and PR exist in the Codeception/phpunit-wrapper repo.

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 10, 2018

Hi! If I understand it correctly you want database connections to be closed automatically using gc_collect_cycles(). If I'm correct you should also unset Yii::$container manually in resetApplication().

Our DB connections are defined in Yii::$container as singletons. They will become unreachable after startApp() has been invoked. But that method is triggered much later and therefore our connections will not be removed by gc_collect_cycles().

Also see; yiisoft/yii2#14997 (comment)

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 10, 2018

If I understand it correctly you want database connections to be closed automatically using gc_collect_cycles().

The issue is more specific than that; references to PDO objects are nulled, but PHP doesn't get around to destroying them (and thus closing database connections).

Our DB connections are defined in Yii::$container as singletons.

If you want to use singletons then you will always get the same database objects and the container keeps a reference to your object. So these objects will never be cleared and you should not have issues.

Why are your database connections singletons?

@DavertMik
Copy link
Member

@DavertMik DavertMik commented Apr 11, 2018

Ok, tests are passing. I'm ok to merge it.
Any objections?

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 11, 2018

None from me!

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 11, 2018

If you want to use singletons then you will always get the same database objects and the container keeps a reference to your object. So these objects will never be cleared and you should not have issues.

startApp() in Codeception\Lib\Connector reconfigures the whole app including the container including the singletons. So our singletons are "nulled" on startApp and then eventually re-instantiated.

Why are your database connections singletons?

It's a trick proposed by @cebe to enable dependency injection of DB via constructor for components. See yiisoft/yii2#14997 (comment).

My suggestion would be to do Yii::$container = null.

@StalkAlex
Copy link

@StalkAlex StalkAlex commented Apr 11, 2018

startApp() in Codeception\Lib\Connector reconfigures the whole app including the container including the singletons. So our singletons are "nulled" on startApp and then eventually re-instantiated.

@erikverheij have you checked last dev version of connector you've mentioned. There is an option now and Yii::$app instance is not recreating anymore by default.

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 11, 2018

startApp() in Codeception\Lib\Connector reconfigures the whole app including the container including the singletons. So our singletons are "nulled" on startApp and then eventually re-instantiated.

https://github.com/Codeception/Codeception/blob/2.4.0/src/Codeception/Lib/Connector/Yii2.php#L63

As far as I can tell, even in 2.4.0, before my changes, the container never got recreated. The container is something that is created when including Yii.php: https://github.com/yiisoft/yii2/blob/master/framework/Yii.php

Recreating the container is not something that should be the default in my opinion.

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 11, 2018

As far as I can tell, even in 2.4.0, before my changes, the container never got recreated. The container is something that is created when including Yii.php: https://github.com/yiisoft/yii2/blob/master/framework/Yii.php. Recreating the container is not something that should be the default in my opinion.

Ah you're right. However the container is being reconfigured when the app is being recreated. https://github.com/yiisoft/yii2/blob/master/framework/base/Application.php#L674

This causes the singletons to be overwritten / inaccessible when singletons are configured using config files.

If we switch to the new version where the app is recreated by default singletons should survive 👍.

However I think that when recreateApplication == true $container should be recreated as well (and therefore also being reset here). In my opinion if you want to recreate the app you want everything to be fresh. From a testers perspective I think it does make sense to always start with a fresh app to make sure app state doesn't effect your tests. Components are also being recreated recreateApplication == true.

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 11, 2018

In my opinion if you want to recreate the app you want everything to be fresh.

I disagree, the app does not own the container.

Components are also being recreated

Yes, because the application owns the components. They are not being explicitly recreated, they are just garbage collected because the application is destroyed and nothing is holding references to the components.

Recreating the DI container could be an optional feature, however it is not even easy to do.
The container is something that gets configured outside of the scope of codeception and the Yii application. It could be in the bootstrap, in your case I guess it's your application config.

Regardless of where it happens these files generally don't get executed more than once and there's no way to recreate the container since we do not know who or what configured it. (This is why the global singleton approach is less than ideal for testing).

@StalkAlex
Copy link

@StalkAlex StalkAlex commented Apr 11, 2018

I agree with @SamMousa. In my case functional tests become broken as I set dependency manually only once before test and then during test I have multiple requests. If during requests within test container will be cleared, then I have to create dependency each amOnPage or form submit instead of once. So container It's something that I want to be fully controlled and I am not expecting from test framework to break injections.

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 11, 2018

The container is something that gets configured outside of the scope of codeception and the Yii application. It could be in the bootstrap, in your case I guess it's your application config.

I don't entirely agree. Indeed, it depends on how you setup the containers. If you do it via config files the Yii application definitely controls the setup of the $container and in my opinion it's therefore too easy to say it's out of the scope of the Yii application.

Personally I think it's better to reset both Yii::$app and Yii::$container (at least the singletons) before each test. This ensures "test A" doesn't affect "test B" through state. Common setup of the $container for each test could be achieved by using some before-each-test-hook. What's the advantage of sharing state across tests? Performance?

Starting every test with a fresh state worked quite well for us until 2.4.0. I think we can work around the suggested approach but it would be nice if "resetting everything" works out of the box. Instead of a "recreateContainer" option I think a "clearContainer" option would more useful (and easier to implement). If it's enabled the container would be set to null. The tester can take the responsibility of repopulating the $container. In our case the app config takes care of it. Together with recreateApplication it's up to the user to decide whether to start with a clean state or to go with a global state.

Thanks for the great discussion guys 👍

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 12, 2018

What's the advantage of sharing state across tests? Performance?

Partially less, things like database preparation take time. Also as long as stuff runs in one process there's always some state (kept in the codeception part).

Starting every test with a fresh state worked quite well for us until 2.4.0.

I checked but as far as I can tell 2.4.0 also didn't clear the container? Note that lots of the issues I introduced where the result of unclear specifications / contracts. So it is not necessarily all by design. The new module however has a clear contract on which you can depend.

Instead of a "recreateContainer" option I think a "clearContainer" option would more useful (and easier to implement).

Well we could do a clean container like we do cleanResponse or cleanRequest.

In our case the app config takes care of it.

The app config doesn't get reloaded for every test though, it's loaded once so if it contains executable code that code is only executed once.

Thanks for the great discussion guys

You too :)

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 12, 2018

I checked but as far as I can tell 2.4.0 also didn't clear the container? Note that lots of the issues I introduced where the result of unclear specifications / contracts. So it is not necessarily all by design. The new module however has a clear contract on which you can depend.

Yeah I think it makes sense. Creating a new app cleared the container for us, but you're right that's not necessarily by design.

The app config doesn't get reloaded for every test though, it's loaded once so if it contains executable code that code is only executed once.

The app config array may contain a "container" entry which causes the container to be reset when the app is being configured. So it's no problem if the config code is only executed once. In the end it results in an array that's being used to setup the app and container.

Well we could do a clean container like we do cleanResponse or cleanRequest.

That would be awesome 👍

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 12, 2018

The app config array may contain a "container" entry which causes the container to be reset when the app is being configured. So it's no problem if the config code is only executed once. In the end it results in an array that's being used to setup the app and container.

This is one of the ugliest design parts of Yii. The goal is to have all config in one file, this configuration option and a few others are very ugly from a application architecture point of view (in my opinion).

That would be awesome +1

Would you care to make a PR?

ps. @DavertMik I think this can be merged; the discussion is not related to this PR

@bscheshirwork
Copy link
Contributor

@bscheshirwork bscheshirwork commented Apr 13, 2018

So... I also catch this

    /**
     * A new client is created for every test, it is destroyed after every test.
     * @see InnerBrowser::_after()
     *
     */
    public function __destruct()
    {
        $this->resetApplication();
    }

called in Rate Limiter filter of rest controller
https://github.com/yiisoft/yii2/blob/abeb3b1a371a3b0babe3fae2d7d6a36765b03bc0/framework/base/Controller.php#L274
https://github.com/yiisoft/yii2/blob/abeb3b1a371a3b0babe3fae2d7d6a36765b03bc0/framework/base/Component.php#L627

beforeFilter

            // call afterFilter only if beforeFilter succeeds
            // beforeFilter and afterFilter should be properly nested
            $this->owner->on(Controller::EVENT_AFTER_ACTION, [$this, 'afterFilter'], null, false);

I vote to merge it now ^^

@erikverheij
Copy link
Contributor

@erikverheij erikverheij commented Apr 13, 2018

@SamMousa
Copy link
Collaborator Author

@SamMousa SamMousa commented Apr 17, 2018

@DavertMik or @samdark I think this can be merged.

The PR @erikverheij is creating is not exclusive with this one.

@samdark samdark merged commit b574ef2 into Codeception:2.4 Apr 17, 2018
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
wercker/build Wercker pipeline passed
Details
@samdark
Copy link
Collaborator

@samdark samdark commented Apr 17, 2018

Merged. Thank you!

@SamMousa SamMousa deleted the SamMousa:manual-app-reset branch Apr 17, 2018
@@ -358,6 +358,8 @@ public function _after(TestInterface $test)
if ($this->client !== null && $this->client->getApplication()->has('session', true)) {
$this->client->getApplication()->session->close();
}

$this->client->resetApplication();

This comment has been minimized.

@wkritzinger

wkritzinger May 27, 2018
Contributor

This should also happen in the null check.
I'd rewrite this block to

if ($this->client !== null) {
    if ($this->client->getApplication()->has('session', true)) {
        $this->client->getApplication()->session->close();
    }
    $this->client->resetApplication();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.