search

realm / realm-core

Core database component for the Realm Mobile Database SDKs
https://realm.io
Apache License 2.0
1.01k stars 155 forks source link

Allow passing `logger` through `RealmConfig` on `SharedRealm::get_shared_realm` #7227

Open kraenhansen opened 8 months ago

kraenhansen commented 8 months ago

Describe your problem or use case

A Node.js process importing the Realm JS SDK currently hangs after completion, because a JS callback is captured as a static when calling realm::util::Logger::set_default_logger.

Describe the solution you'd like

We'd like to instead pass a logger in config when calling SharedRealm::get_shared_realm, to make the logger passed to and owned by the underlying DB here: https://github.com/realm/realm-core/blob/4926f59b1dddb6b84568eaf2d9bc8a53c46437de/src/realm/object-store/impl/realm_coordinator.cpp#L474

Ideally we'd like to extend this pattern to the SyncManager via the SyncClientConfig, so our users don't have to call set_logger_factory after construction of the Realm. In its current state it takes a "logger factory", not the logger inself. https://github.com/realm/realm-core/blob/4926f59b1dddb6b84568eaf2d9bc8a53c46437de/src/realm/object-store/sync/sync_manager.hpp#L79

Additional context

Ideally, from the SDK's point of view, we'd like to just pass in a lambda (std::function<void(util::Logger::Level, const std::string& message)>) through the config - instead of a full-fledged Logger instance.

nirinchev commented 8 months ago

I don't have any knowledge of the internals of node.js, so apologies if this is a silly question, but wouldn't that just push the problem around? I.e. if I open a Realm file and don't explicitly close it, wouldn't Core keep a captured JS callback for the logger, preventing the process from exiting? Or are we relying on the GC to kick in and eventually collect the Realm instance and free up the callback?

kraenhansen commented 8 months ago

relying on the GC to kick in and eventually collect the Realm instance and free up the callback?

Exactly, GC would free the Realm instance, which would ultimately free the JS function. But even if it did require an explicit Realm#close (which is expected if any listeners are registered), it'd be much better than what we have now.

nirinchev commented 8 months ago

We have a similar (though not identical) problem with dart console apps where we added Realm.shutdown method that developers can call at the end of a dart program to allow clean exit of the process. Not sure if that's a path that makes sense for JS as well.

The problem with allowing the SDK to supply loggers for each component is that it moves us further from the goal of having a single globally-available logger (as defined in the first goal of this scope doc) and increases the complexity of Core having to manage multiple logger configuration sources. Not saying that it's impossible or particularly difficult to do, but it's extra complexity and we should weigh that vs the benefits.