Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to close DuckDB database connections, addressing file lock issues when multiple programs need to access the same database. The implementation introduces a global registry to track connection pools and provides methods to explicitly close connections.
- Adds
close_duckdb_connections()function incommon.pyto close connection pools for DuckDB databases - Implements
close_db_connections()method inmaplibregl.Mapto close connections for databases used by a specific map instance - Tracks database paths in
_duckdb_databaseslist to enable per-map connection management
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| leafmap/maplibregl.py | Adds database path tracking and close_db_connections() method to Map class for managing DuckDB connection lifecycle |
| leafmap/common.py | Implements global connection pool registry and close_duckdb_connections() function for closing database connections |
| docs/maplibre/duckdb_layer.ipynb | Adds example calls to close_db_connections() after using DuckDB layers; updates Python version metadata |
| .gitignore | Adds *.zip and *.db file patterns to ignore database and archive files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| traceback.print_exc() | ||
|
|
||
| def close_db_connections(self, database_path: str = None, quiet: bool = False): |
There was a problem hiding this comment.
[nitpick] The quiet parameter defaults to False in close_db_connections, but it defaults to True in the underlying close_duckdb_connections function in common.py. This inconsistency could be confusing. Consider aligning the default values for consistency, or document why they differ.
| # Close all connections in the pool | ||
| closed_count = 0 | ||
| while not connection_pool.empty(): | ||
| try: | ||
| con = connection_pool.get_nowait() | ||
| con.close() | ||
| closed_count += 1 | ||
| except Exception as e: | ||
| if not quiet: | ||
| print(f"Error closing connection: {e}") | ||
|
|
There was a problem hiding this comment.
The connection closing logic only closes connections that are currently in the pool (idle connections). If connections are currently being used by active requests (checked out from the pool), they won't be closed. Consider adding logic to wait for active connections to be returned or documenting this limitation in the function docstring.
| # Remove from registry | ||
| del _duckdb_connection_pools[db_path] |
There was a problem hiding this comment.
After removing the database from the registry, the tile server Flask app that was started in start_duckdb_tile_server will continue running in its daemon thread but will no longer have access to connections. This could lead to errors when the server tries to serve tiles. Consider documenting that the tile server needs to be manually stopped or restarted after closing connections.
| if db_path not in self._duckdb_databases: | ||
| self._duckdb_databases.append(db_path) |
There was a problem hiding this comment.
The database path tracking uses a list with a membership check, which has O(n) time complexity. For better performance with multiple databases, consider using a set instead:
self._duckdb_databases = set() # in __init__
# Then use:
self._duckdb_databases.add(db_path)This would also automatically prevent duplicates without the explicit check.
|
🚀 Deployed on https://691742c10bc0e05928e8c880--opengeos.netlify.app |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Add close_db_connections method * Update leafmap/common.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update leafmap/common.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update leafmap/maplibregl.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.