Since database connections are stored as static variables, they cannot be reconnected to. This is an efficient way of handling them, but developers should have the option to reconnect if necessary. A specific case where this is required is illustrated in the Cron Multi-Threaded module. Since it utilizes the PCNTL extension to fork the process and run cron hooks in parallel, the default database connection is closed when the first child process exits requiring a reconnect in the parent process. The fix is adding a third parameter to the DatabaseConnection::getConnection() method giving developers the option to force a reconnect. It defaults to FALSE, so the current functionality in not altered in any way.

CommentFileSizeAuthor
#21 database.inc-close-db-test-02.patch2.92 KBcpliakas
#18 database.inc-close-db-test-01.patch3.68 KBcpliakas
#17 database.inc-close-db-test.patch3.58 KBcpliakas
#12 database.inc-close-db.patch1.74 KBcpliakas
database.inc-reconnect.patch1.32 KBcpliakas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Revision to the previous post... the change is made to the Database::getConnection() method, not the DatabaseConnection::getConnection() method which obviously does not exist :-).

webkenny’s picture

webkenny CreditAttribution: webkenny commented

+1

sethcohn’s picture

sethcohn CreditAttribution: sethcohn commented

+1, seems important for scalability in the future

MMachnik’s picture

MMachnik CreditAttribution: MMachnik commented

+1 adds support for multi-threaded functionality.

ebeyrent’s picture

ebeyrent CreditAttribution: ebeyrent commented

+1 here too...

Crell’s picture

Crell CreditAttribution: Crell commented

Hm. Do we want to have a reconnect flag, or an explicit close operation that unsets the connection object? It would then reconnect on the next call.

I think I'd prefer an explicit closeConnection(), as that has other potential uses as well.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Hi Crell.

Great work on the D7 abstraction layer, and I enjoyed your presentation in DC. My thinking with the reconnect flag is that it is a terse solution to the problem with very little code change. My only concern with the closeConnection() method is that developers would have to start testing the state of the database connection, but if a connection is re-established automatically on the next call without the developer having to explicitly reconnect, then it isn't a problem. Either way, I agree that a closeConnection() method does have other potential uses, and I like the idea. The only requirement for my purposes is that the new connection uses a completely new resource ID, which is accomplished with the unset of the connection's static variable.

Thanks again for your hard work. PDO in core is a huge step forward for Drupal.

Crell’s picture

Crell CreditAttribution: Crell commented
Title: Parameter in DatabaseConnection::getConnection() to allow developers to reconnect to the database. » Add Database::closeConnection() to explicitly close a connection

getConnection() already handles lazy connection by design, so that shouldn't be a problem. Really, the correct way to handle a refresh would be to close the connection first and then let it reopen on next request, which requires a closeConnection() primitive operation anyway. So let's add a closeConnection() operation, and then later if we find that we need a reset parameter as well we can add it then. I suspect it won't be necessary, though.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work

I agree with Crell that a closeConnection() method is probably better as it allows single-threaded code to free resources too. It never hurts. It makes for better API design too, IMO.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Thanks for the discussion. What you are describing does eliminate the need for the reconnect flag, and it addresses my specific issue. I am willing to contribute a patch to add this functionality, but if you want to handle it that is fine too. Thanks again.

Crell’s picture

Crell CreditAttribution: Crell commented

@cpliakas: Please do! The more people get involved in Drupal development the better for everyone. :-)

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
FileSize
database.inc-close-db.patch1.74 KB

Agreed :-) Here is my attempt at the solution. I added the Database::closeConnection() method, and I also created a db_close() function to expand upon the API. Both allow the developer to close the connection as well as specific targets. Similar to the other db_*() functions, db_close() acts on the active database. I couldn't think of a meaningful value to return, so any guidance would be appreciated.

Crell’s picture

Crell CreditAttribution: Crell commented

Thanks! Comments follow.

- The other wrapping functions use isset() or empty(), not array_key_exists(). array_key_exists() is slower than the other two, so let's follow the same pattern as the other functions.

- Code looks good to me, and is well documented. We need a unit test, though. I'm not entirely certain the best way to do that, but there is already a test class for connection-related tests that you should add to. Unfortunately connection management is difficult to unit test because of the statics. :-(

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Thanks for the feedback. I am all about compliance with patterns, so I will replace the array_key_exists() function. I will also experiment with the unit tests and will submit another patch for review.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

I am still alive :-). Haven't had much time to work on my personal projects, but I will get to this soon.

Crell’s picture

Crell CreditAttribution: Crell commented

cpliakas, are you still working on this? If not, does someone else want to pick it up? I think it's an important addition.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
FileSize
database.inc-close-db-test.patch3.58 KB

Hi Crell.

Sorry for the delay. I promise to be more attentive to this thread as the code freeze is drawing near. Here is my attempt at the test. I think it works as commenting out the closeConnection() call makes the test fail. Please advise if there is a better way to go about this.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
FileSize
database.inc-close-db-test-01.patch3.68 KB

After submitting the patch, I realized some of the comments were lacking. This revised patch adds some better comments.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Needs review

Remember to set an issue to "needs review" so that the bot will check it.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Needs work

You need to remove the htaccess change that snuck into the patch. As noted in the comment, the lack of closing quote is there for a reason.

The logic of the unit test looks fine. However, it should be its own test method. Don't just tuck another test into an existing method. Good unit test methods are small and single-purpose.

The logic itself looks fine aside from those issues. Should be an easy update.

Thanks!

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs work » Needs review
FileSize
database.inc-close-db-test-02.patch2.92 KB

Sorry about the .htaccess file. It's a bad habit to add the quote, and I forgot to omit it from the patch. Here is another crack at the test. There also seems to be a superfluous check in db_close(), so I removed it for consistency with the other functions. Thanks for your feedback.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

Of note, we usually default the target to "default", not NULL, in the factory function. However, I think it's reasonable to default to NULL here on the grounds that most of the time if we're closing a connection to a database, we really do want to close all related connections, including master and slave servers (if applicable). The bot likes it, and so do I. :-)

Thanks, cpliakas!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Well done! This looks great and makes the API more robust.

Committed to HEAD.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Awesome! Thanks again for the guidance.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.