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

bundles that open resources should close them on shutdown() #2672

Closed
lsmith77 opened this Issue Nov 18, 2011 · 7 comments

Comments

Projects
None yet
7 participants
@lsmith77
Contributor

lsmith77 commented Nov 18, 2011

For example the MonologBundle, but also all Database bundles etc.
This of course also relates to any non core Bundles

Aka the Bundles need to override the default shutdown() methods
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Bundle/Bundle.php#L44

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77
Contributor

lsmith77 commented Nov 18, 2011

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 18, 2011

Member

@lsmith77 the issue for databases is that PDO does not allow closing the connection. The way to close it is to garbage collect the PDO instance, which is a bit hard if someone starts to reference the wrapped connection of DBAL (which is not the case by default). But the worse case is someone creating a PDO service in the container as it would mean destructing all services depending on it.

And I see another point: closing all connections in shutdown means that we will have to create them even if the request has not instantiated the service yet as there is no way to know about it.

Member

stof commented Nov 18, 2011

@lsmith77 the issue for databases is that PDO does not allow closing the connection. The way to close it is to garbage collect the PDO instance, which is a bit hard if someone starts to reference the wrapped connection of DBAL (which is not the case by default). But the worse case is someone creating a PDO service in the container as it would mean destructing all services depending on it.

And I see another point: closing all connections in shutdown means that we will have to create them even if the request has not instantiated the service yet as there is no way to know about it.

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Nov 18, 2011

Contributor

I had an discussion over this with marc the other day, since in functional
tests you will hit the max connections limit very fast. However as Stof
said before, PDO is completely useless with regard to closing connections.
Doctrine\DBAL\Connection::close() did not help marc at all to solve this
problem. This is very annoying.

Also the problem with not knowing the services that are instantiated is
another problem here.

But i agree that there is need for something like this.

On Fri, Nov 18, 2011 at 10:14 AM, Christophe Coevoet <
reply@reply.github.com

wrote:

@lsmith77 the issue for databases is that PDO does not allow closing the
connection. The way to close it is to garbage collect the PDO instance,
which is a bit hard if someone starts to reference the wrapped connection
of DBAL (which is not the case by default). But the worse case is someone
creating a PDO service in the container as it would mean destructing all
services depending on it.

And I see another point: closing all connections in shutdown means that we
will have to create them even if the request has not instantiated the
service yet as there is no way to know about it.


Reply to this email directly or view it on GitHub:
#2672 (comment)

Contributor

beberlei commented Nov 18, 2011

I had an discussion over this with marc the other day, since in functional
tests you will hit the max connections limit very fast. However as Stof
said before, PDO is completely useless with regard to closing connections.
Doctrine\DBAL\Connection::close() did not help marc at all to solve this
problem. This is very annoying.

Also the problem with not knowing the services that are instantiated is
another problem here.

But i agree that there is need for something like this.

On Fri, Nov 18, 2011 at 10:14 AM, Christophe Coevoet <
reply@reply.github.com

wrote:

@lsmith77 the issue for databases is that PDO does not allow closing the
connection. The way to close it is to garbage collect the PDO instance,
which is a bit hard if someone starts to reference the wrapped connection
of DBAL (which is not the case by default). But the worse case is someone
creating a PDO service in the container as it would mean destructing all
services depending on it.

And I see another point: closing all connections in shutdown means that we
will have to create them even if the request has not instantiated the
service yet as there is no way to know about it.


Reply to this email directly or view it on GitHub:
#2672 (comment)

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Nov 18, 2011

Member

Here's what I did for functional tests, not nice but it helped.

    static protected function createClient(array $options = array(), array $server = array())
    {
        if (!static::$kernel) {
            self::$kernel = self::createKernel($options);
            self::$kernel->boot();
            self::$conn = self::$kernel->getContainer()->get('doctrine.dbal.default_connection');
        } else {
            self::$kernel->boot();
            self::$kernel->getContainer()->set('doctrine.dbal.default_connection', self::$conn);
        }

        $client = self::$kernel->getContainer()->get('test.client');
        $client->setServerParameters($server);

        return $client;
    }
Member

Seldaek commented Nov 18, 2011

Here's what I did for functional tests, not nice but it helped.

    static protected function createClient(array $options = array(), array $server = array())
    {
        if (!static::$kernel) {
            self::$kernel = self::createKernel($options);
            self::$kernel->boot();
            self::$conn = self::$kernel->getContainer()->get('doctrine.dbal.default_connection');
        } else {
            self::$kernel->boot();
            self::$kernel->getContainer()->set('doctrine.dbal.default_connection', self::$conn);
        }

        $client = self::$kernel->getContainer()->get('test.client');
        $client->setServerParameters($server);

        return $client;
    }
@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Nov 18, 2011

Contributor

I experimented some issues in the Propel2 test suite, a call to Propel::close() in tearDownAfterClass methods solved them.

I can add a shutdown() method in the PropelBundle and, then call Propel::close(). As Propel manages connections itself and the singleton is instanciated all the time, I think I'm not concerned by @stof's last sentence.

Contributor

willdurand commented Nov 18, 2011

I experimented some issues in the Propel2 test suite, a call to Propel::close() in tearDownAfterClass methods solved them.

I can add a shutdown() method in the PropelBundle and, then call Propel::close(). As Propel manages connections itself and the singleton is instanciated all the time, I think I'm not concerned by @stof's last sentence.

@videlalvaro

This comment has been minimized.

Show comment
Hide comment
@videlalvaro

videlalvaro Nov 18, 2011

on our tearDown method we do something like:

    public function tearDown()
    {
      ...
      $this->shutdownServices(static::$kernel->getContainer());
      ...
    }
    $this->shutdownServices(static::$kernel->getContainer());

     protected function shutdownServices($container)
    {
            $container->get('doctrine_phpcr.default_session')->logout();
            $container->get('monolog.handler.main')->close();
    }

Still I think this has to be taken care by the framework or the bundles.

videlalvaro commented Nov 18, 2011

on our tearDown method we do something like:

    public function tearDown()
    {
      ...
      $this->shutdownServices(static::$kernel->getContainer());
      ...
    }
    $this->shutdownServices(static::$kernel->getContainer());

     protected function shutdownServices($container)
    {
            $container->get('doctrine_phpcr.default_session')->logout();
            $container->get('monolog.handler.main')->close();
    }

Still I think this has to be taken care by the framework or the bundles.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 13, 2012

Member

Closing this ticket as this is related to Doctrine.

Member

fabpot commented Feb 13, 2012

Closing this ticket as this is related to Doctrine.

@fabpot fabpot closed this Feb 13, 2012

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