OHDSI / ResultModelManager

RMM is an R package designed to handle common ohdsi results data management functions by providing a common API for data model migrations and definitions
https://ohdsi.github.io/ResultModelManager/
Apache License 2.0
3 stars 3 forks source link

Incorrect Handling of Database Pool in getConection Causes launchDiagnosticsExplorer to Fail #62

Closed mccullen closed 1 month ago

mccullen commented 1 month ago

I noticed the PooledConnectionHandler getConnection function was incorrectly returning the pool object itself instead of a specific connection from within the pool. This leads to a failure in CohortDiagnoistics::launchDiagnosticsExplorer, as the expected DB connection type checks within DatabaseConnector::getTableNames were never satisfied due to receiving a pool object instead of a valid connection.

To address this issue locally, I made the following changes: https://github.com/OHDSI/ResultModelManager/compare/main...mccullen:ResultModelManager:main

Edit: Interestingly, this was not a problem in my container environment, just my local, not sure why, but then I got a different error.

azimov commented 1 month ago

@mccullen I should have recently fixed a related issue in OhdsiShinyModules, however I appreciate your changes as they simplify the process. I will attempt to roll these in to the next release

azimov commented 1 month ago

On second thoughts the problem with "getConnection" returning is a connection is that the pooled object is left hanging, without this closing. This can leave a bunch of checked out pooled objects which will result in multi user shiny apps crashing (the reason we use pooled connections in the first place is why this happens).

I suspect that there is a better solution where the class masks a call to DatabaseConnector getTableNames and this can be used downstream in shiny apps.

I will work on an alternative solution using withr::defer that forces a poolReturn call when calling getConnection inside the parent frame

mccullen commented 1 month ago

Great. Thank you. Sure, that sounds good.