[replit] The lesser of two evils

[lhchavez]

Why

Currently the replit library has a very gross quirk: it has a global in replit.database.default_db.db, and the mere action of importing this library causes side effects to run! (connects to the database, starts a thread to refresh the URL, and prints a warning to stdout, adding insult to injury).

What changed

So we're trading that very gross quirk with a gross workaround to preserve backwards compatibility: the modules that somehow end up importing that module now have a __getattr__ that lazily calls the code that used to be invoked as a side-effect of importing the library. Maybe in the future we'll deploy a breaking version of the library where we're not beholden to this backwards-compatibility quirck.

Test plan

replit-py/src$ python3
Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import replit
>>> from replit.database import db
WARNING:root:Warning: error initializing database. Replit DB is not configured.
>>> repr(db)
'None'
>>> 

Rollout

• This is fully backward and forward compatible

[airportyh]

lgtm

[blast-hardcheese]

Hah! I started to write this very changeset. Is there any more testing to do here @lhchavez, or do you want to rebase and we can get this in and cut a minor release?

[blast-hardcheese]

I made some changes:

• reintroduced a stub for the refresh_db() method
• Added get_db() and get_db_url() classmethods on LazyDB to avoid LazyDB.get_instance().db/.db_url pattern. That's not an API surface I would feel comfortable maintaining long term.

[blast-hardcheese]

Continuation from #167 (comment),

The more I looked at LazyDB the more I disliked how we had two halves of a Database class, plus lots of singleton indirection threaded through... it wasn't pleasant, and would almost certainly not be stable as we continue to iterate.

I reverted the surface area to what it was before, save with one alteration.

Internally, we only ever use the __getattr__ indirection, avoiding holding a binding all the way up to default_db, which holds its binding in _db.
If users decide to maintain a reference to db in their own code, this is not an issue, since the whole point here was to defer connection until the user required it.

Another issue was that of close(). I now pass unbind=... as a parameter to Database and AsyncDatabase such that when their close() methods are called, the _db global in default_db is cleared. The next time replit.db is accessed, it will reinitialize and reconnect, but this permits a less clunky resource management story.

Finally, a complaint I've seen around is that the Python interpreter will hang if the database has been connected to, since we threading.Timer(...).start(), throwing away the intermediate object that had the corresponding .cancel(). Instead, Database and AsyncDatabase can maintain their own DB URLs, if they receive a get_db_url callable passed in their initializers.

This was the summary of trying to get some thoughts out of my head before the weekend, I still need to revisit the tests, add missing documentation, and probably rename unbind to cleanup or post_cancel or something less prescriptive.

[blast-hardcheese]

@airportyh Dismissed your initial review since I'm taking a different approach from Luis' initial implementation.

The changes here attempt to maximize compatibility with the existing API without adding too much to the surface area of the library.

There's an additional improvement which was not reflected in the original implementation, being that when the Database is closed, the threading.Timer is closed and the default db property is reset back to None. This means the subsequent accesses of that property will reestablish and reconnect the database object automatically.

[airportyh]

Is it even possible to use a db other than the default_db?

[blast-hardcheese]

Yes! As an example, we use it in some CI tests internally. An example is as follows (I added a print in the default DB initializer):

>>> import os
>>> from replit.database import Database
>>> db = Database(os.environ.get("REPLIT_DB_URL"))
>>> db.set('foo', 'bar')
>>> db.get('foo')
'bar'
>>> import replit
>>> replit.db
Initializing default db!
<Database(db_url=...)>

You can see in this example that the initializer for the default database is only triggered on certain property accessors.

[airportyh]

Thanks for the cleanup!