Page MenuHomePhabricator

LoadBalancer opening extra connections in different connection categories doesn't work with PHPUnit & temporary tables
Closed, ResolvedPublic

Description

This is a followup to T201137: WikibaseLexeme 'jenkins_u0_mw.unittest_content_models' doesn't exist.

While running phpunit with temporary tables, the temporary tables will only exist on a single DB connection.
LoadBalancer::openLocalConnection can open extra connections if the auto commit flag is passed in and thus a connection category / connKey of "localAutoCommit" is used instead of "local".
In cases where this happens the new connection will not have access to the temporary tables created on the initial connection.

Hopefully now this issue is on phabricator if someone runs into it again they'll spot this and quickly figure out what is happening.

CCing @aaron as he probably has an idea of what is best.

Possible ways forward

  • In unit tests when a local connection with autocommit is loaded, instead load the same connection that already exists?
  • In unit tests when a local connection with autocommit is loaded, instead fail HARD, throw an exception that makes sense. (something like https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/450524/ perhaps)
  • Stop using temporary tables in unit tests & CI in general? (Adding @hashar for this idea specifically)

Event Timeline

Addshore created this task.Aug 17 2018, 7:22 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 17 2018, 7:22 AM
Addshore updated the task description. (Show Details)Aug 17 2018, 8:03 AM

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

I don't think that is quite the same as this issue.
This issue talks specifically about the connection groups within a single LoadBalancer but all pointing to the same underlying DB.
Your issue sounds like an AbuseFilter issue, perhaps in the abuse filter tests a new connection should not be used, just pass in the connection being used in the tests.

I was going to open a new task for this problem and noticed this task just before pressing "Create New task". This is the paste of what I was going to say:

While writing unit tests for AbuseFilter, I came to testing global filters, which are stored in the DB of a central wiki and executed in other wikis. The code to test instantiates a LoadBalancer instance to connect to the foreign database (here), but when this happens inside a unit test the prefix 'unittest_' is gone. Although I don't know DB-related classes really well, I can say that this happens due to LoadBalancer::openForeignConnection, which sets up a connection with no prefix. For reference, this is the patch to add the unit tests in question. There may be an error in there, but losing prefixes shouldn't really happen and I'm opening this task to be sure.

So my assumption seems to be almost correct, and this is actually a core problem. If, instead, I'm not right, then I'll open a separate task for the issue.

I don't think that is quite the same as this issue.
This issue talks specifically about the connection groups within a single LoadBalancer but all pointing to the same underlying DB.
Your issue sounds like an AbuseFilter issue, perhaps in the abuse filter tests a new connection should not be used, just pass in the connection being used in the tests.

In fact, reading the task description again, it seems like a different issue. However, in the AF case, the two connections are still pointing to the same DB (for convenience), so this led me to thinking it actually was the same. While I'm not sure that the test connection can be passed to the actual code, it'd be a partial relief to know that this is not a core problem.

Krinkle moved this task from Inbox to PHPUnit on the MediaWiki-Core-Testing board.Oct 5 2018, 8:20 PM
Krinkle added a subscriber: Krinkle.

This area of wikimedia/rdbms has been significantly refactored by @aaron in recent months. Tagging our workboard for him to triage when he's back. This may've been fixed already.

Restricted Application added a project: Core Platform Team. · View Herald TranscriptJul 23 2019, 6:03 PM
Krinkle triaged this task as Normal priority.Jul 23 2019, 6:04 PM
Krinkle removed a project: Core Platform Team.
Krinkle assigned this task to aaron.Aug 6 2019, 5:56 PM

Change 530982 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] rdbms: add setTempTablesOnlyMode() to suppress CONN_TRX_AUTOCOMMIT during tests

https://gerrit.wikimedia.org/r/530982

hashar removed a subscriber: hashar.Aug 20 2019, 8:09 AM

Change 530982 merged by jenkins-bot:
[mediawiki/core@master] rdbms: add setTempTablesOnlyMode() to suppress CONN_TRX_AUTOCOMMIT during tests

https://gerrit.wikimedia.org/r/530982

aaron closed this task as Resolved.Aug 25 2019, 9:37 PM