Skip to content

Commit 5b84e36

Browse files
wilhuffjonsimantov
authored andcommitted
Allow Firestore::set_log_level to be called early on Android
Handle calls to Firestore::set_log_level before any Firestore instances have been created. On Android, defer calls to FirebaseFirestore.setLogLevel until after an instance has been created. PiperOrigin-RevId: 348025209
1 parent 069e4c4 commit 5b84e36

File tree

3 files changed

+63
-33
lines changed

3 files changed

+63
-33
lines changed

firestore/src/android/firestore_android.cc

Lines changed: 60 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -178,17 +178,24 @@ class JavaFirestoreMap {
178178
jni::Global<jni::HashMap> firestores_;
179179
};
180180

181-
// Governed by init_mutex_below as well.
182-
JavaFirestoreMap* java_firestores_ = nullptr;
181+
// init_mutex protects all the globals below.
182+
Mutex init_mutex; // NOLINT
183+
int initialize_count = 0;
184+
Loader* global_loader = nullptr;
185+
186+
JavaFirestoreMap* java_firestores = nullptr;
187+
188+
// The initial value for setLoggingEnabled.
189+
enum class InitialLogState {
190+
kUnset,
191+
kSetEnabled,
192+
kSetDisabled,
193+
} initial_log_state = InitialLogState::kUnset;
183194

184195
} // namespace
185196

186197
const char kApiIdentifier[] = "Firestore";
187198

188-
Mutex FirestoreInternal::init_mutex_; // NOLINT
189-
int FirestoreInternal::initialize_count_ = 0;
190-
Loader* FirestoreInternal::loader_ = nullptr;
191-
192199
FirestoreInternal::FirestoreInternal(App* app) {
193200
FIREBASE_ASSERT(app != nullptr);
194201
if (!Initialize(app)) return;
@@ -200,7 +207,7 @@ FirestoreInternal::FirestoreInternal(App* app) {
200207
FIREBASE_ASSERT(java_firestore.get() != nullptr);
201208
obj_ = java_firestore;
202209

203-
java_firestores_->Put(env, java_firestore, this);
210+
java_firestores->Put(env, java_firestore, this);
204211

205212
// Mainly for enabling TimestampsInSnapshotsEnabled. The rest comes from the
206213
// default in native SDK. The C++ implementation relies on that for reading
@@ -218,12 +225,12 @@ FirestoreInternal::FirestoreInternal(App* app) {
218225

219226
/* static */
220227
bool FirestoreInternal::Initialize(App* app) {
221-
MutexLock init_lock(init_mutex_);
222-
if (initialize_count_ == 0) {
228+
MutexLock init_lock(init_mutex);
229+
if (initialize_count == 0) {
223230
jni::Initialize(app->java_vm());
224231

225-
FIREBASE_DEV_ASSERT(java_firestores_ == nullptr);
226-
java_firestores_ = new JavaFirestoreMap();
232+
FIREBASE_DEV_ASSERT(java_firestores == nullptr);
233+
java_firestores = new JavaFirestoreMap();
227234

228235
Loader loader(app);
229236
loader.AddEmbeddedFile(::firebase_firestore::firestore_resources_filename,
@@ -272,32 +279,43 @@ bool FirestoreInternal::Initialize(App* app) {
272279
TransactionInternal::Initialize(loader);
273280
WriteBatchInternal::Initialize(loader);
274281
if (!loader.ok()) {
275-
ReleaseClasses(app);
282+
ReleaseClassesLocked(app);
276283
return false;
277284
}
278285

279-
FIREBASE_DEV_ASSERT(loader_ == nullptr);
280-
loader_ = new Loader(Move(loader));
286+
FIREBASE_DEV_ASSERT(global_loader == nullptr);
287+
global_loader = new Loader(Move(loader));
288+
289+
if (initial_log_state != InitialLogState::kUnset) {
290+
Env env;
291+
bool enabled = initial_log_state == InitialLogState::kSetEnabled;
292+
env.Call(kSetLoggingEnabled, enabled);
293+
294+
// If this fails it's unimportant. If there's some broad systemic failure
295+
// the next real operation will trigger it too.
296+
env.ExceptionClear();
297+
}
281298
}
282-
initialize_count_++;
299+
initialize_count++;
283300
return true;
284301
}
285302

286303
/* static */
287-
void FirestoreInternal::ReleaseClasses(App* app) {
288-
delete loader_;
289-
loader_ = nullptr;
304+
void FirestoreInternal::ReleaseClassesLocked(App* app) {
305+
// Assumes `init_mutex` is held.
306+
delete global_loader;
307+
global_loader = nullptr;
290308
}
291309

292310
/* static */
293311
void FirestoreInternal::Terminate(App* app) {
294-
MutexLock init_lock(init_mutex_);
295-
FIREBASE_ASSERT(initialize_count_ > 0);
296-
initialize_count_--;
297-
if (initialize_count_ == 0) {
298-
ReleaseClasses(app);
299-
delete java_firestores_;
300-
java_firestores_ = nullptr;
312+
MutexLock init_lock(init_mutex);
313+
FIREBASE_ASSERT(initialize_count > 0);
314+
initialize_count--;
315+
if (initialize_count == 0) {
316+
ReleaseClassesLocked(app);
317+
delete java_firestores;
318+
java_firestores = nullptr;
301319
}
302320
}
303321

@@ -316,7 +334,7 @@ FirestoreInternal::~FirestoreInternal() {
316334

317335
future_manager_.ReleaseFutureApi(this);
318336

319-
java_firestores_->Remove(env, obj_);
337+
java_firestores->Remove(env, obj_);
320338

321339
Terminate(app_);
322340
app_ = nullptr;
@@ -512,13 +530,28 @@ void Firestore::set_log_level(LogLevel log_level) {
512530
// "Info", "warning", "error", and "assert" map to logging disabled.
513531
bool logging_enabled = log_level < LogLevel::kLogLevelInfo;
514532

533+
{
534+
MutexLock lock(init_mutex);
535+
536+
// Set the initial_log_state on every invocation, just in case Firestore
537+
// is terminated for long enough to unload the Firestore classes within
538+
// the JVM.
539+
initial_log_state = logging_enabled ? InitialLogState::kSetEnabled
540+
: InitialLogState::kSetDisabled;
541+
542+
if (initialize_count < 1) {
543+
// Avoid invoking Java methods before Firestore has been initialized.
544+
return;
545+
}
546+
}
547+
515548
Env env = FirestoreInternal::GetEnv();
516549
env.Call(kSetLoggingEnabled, logging_enabled);
517550
}
518551

519552
FirestoreInternal* FirestoreInternal::RecoverFirestore(
520553
Env& env, const Object& java_firestore) {
521-
return java_firestores_->Get(env, java_firestore);
554+
return java_firestores->Get(env, java_firestore);
522555
}
523556

524557
void FirestoreInternal::SetClientLanguage(const std::string& language_token) {

firestore/src/android/firestore_android.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -178,13 +178,9 @@ class FirestoreInternal {
178178
void ShutdownUserCallbackExecutor(jni::Env& env);
179179

180180
static bool Initialize(App* app);
181-
static void ReleaseClasses(App* app);
181+
static void ReleaseClassesLocked(App* app);
182182
static void Terminate(App* app);
183183

184-
static Mutex init_mutex_;
185-
static int initialize_count_;
186-
static jni::Loader* loader_;
187-
188184
jni::Global<jni::Object> user_callback_executor_;
189185

190186
App* app_ = nullptr;

firestore/src/tests/firestore_integration_test.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ int WaitFor(const FutureBase& future) {
101101
FirestoreIntegrationTest::FirestoreIntegrationTest() {
102102
// Allocate the default Firestore eagerly.
103103
TestFirestore();
104-
Firestore::set_log_level(LogLevel::kLogLevelDebug);
105104
}
106105

107106
Firestore* FirestoreIntegrationTest::TestFirestore(
@@ -118,6 +117,8 @@ Firestore* FirestoreIntegrationTest::TestFirestore(
118117
apps_[app] = UniquePtr<App>(app);
119118
}
120119

120+
Firestore::set_log_level(LogLevel::kLogLevelDebug);
121+
121122
Firestore* db = new Firestore(CreateTestFirestoreInternal(app));
122123
firestores_[db] = FirestoreInfo(name, UniquePtr<Firestore>(db));
123124

0 commit comments

Comments
 (0)