Do not return uninitialized box

[maRci002]

#846 fix should be rerolled.

_boxes[name] = newBox;
await newBox.initialize();

_boxes can contain a box which isn't initialized so it is possible that isBoxOpen(name) return true and program can acquire this uninitialized box which can lead serios problems for example box.put called.

Imagine the following situation:

var box = Hive.openBox('box');
var sameBox = Hive.openBox('box');

In this case sameBox can be referenced before it's initialized. However this cannot be achieved so easily because we have to get the reference after _boxes[name] = newBox; and during await newBox.initialize(); call (and also after _manager.open).

There is a period when both isBoxOpen(name) and _openingBoxes.containsKey(name) return true.
Let me a show an example (I won’t be bored with how the dart event loop works, this is the easiest example):

import 'dart:async';
import 'dart:io';

import 'package:hive/hive.dart';
import 'package:hive/src/backend/storage_backend.dart';

import 'package:hive/src/hive_impl.dart';

const boxName = 'test_box';

final completer = Completer();

final f1 = Completer();
final f2 = Completer();

class BackendManagerInterceptor extends BackendManager {
  @override
  Future<StorageBackend> open(
      String name, String? path, bool crashRecovery, HiveCipher? cipher) async {
    final result = await super.open(name, path, crashRecovery, cipher);

    completer.complete();
    return result;
  }
}

void main() async {
  final sep = Platform.pathSeparator;
  final dbDirectory = Directory('${Directory.current.path}${sep}hive_db');

  print('dbDirectory: ${dbDirectory.path}');
  await resetAndInitDatabaseLocationPath(dbDirectory);

  final hive = HiveImpl.debug(BackendManagerInterceptor());
  hive.init(dbDirectory.path);

  var box = hive.openBox(boxName);
  box.then(
    (value) {
      print('Successful init');
      f1.complete();
    },
  );

  completer.future.then((value) {
    var sameBox = hive.openBox(boxName);
    sameBox.then(
      (value) {
        print('Box is avaible before it\'s initialized');
        f2.complete();
      },
    );
  });

  // make sure program doesn't exit before all print executed
  await (Future.wait([f1.future, f2.future]));
}

Future<void> resetAndInitDatabaseLocationPath(Directory dbDirectory) async {
  if (await dbDirectory.exists()) {
    await dbDirectory.delete(recursive: true);
  }
  await dbDirectory.create(recursive: true);
}

There is another fix just place _openingBoxes.containsKey(name) check before _boxes[name] = newBox; however I think there shouldn't be any point in time when both return true, moreover in case of error _boxes will contain an unitialized box.

#846 should use Hive.deleteBoxFromDisk('boxName'); which handles the delete of unopened box ( see implementation ).

  @override
  Future<void> deleteBoxFromDisk(String name, {String? path}) async {
    var lowerCaseName = name.toLowerCase();
    var box = _boxes[lowerCaseName];
    if (box != null) {
      await box.deleteFromDisk();
    } else {
      await _manager.deleteBox(lowerCaseName, path ?? homePath);
    }
  }

[maRci002]

In nutshell if some one calls Hive.openBox while await newBox.initialize(); is working it is possible to acquire an uninitialized box.

[themisir]

Yes that's correct. But there's another issue with your proposed solution. Check #846 for additional details. TLDR if initialization fails db file keeps locked on WindowsOS, so you can't delete or re-open said box.

[themisir]

Also when you re-call openBox it will not return uninitialized box because we have some sort of locking mechanism in place that will await openBox calls to same box while initialization completes.

https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L82-L89

[themisir]

#846 should use Hive.deleteBoxFromDisk('boxName'); which handles the delete of unopened box ( see implementation ).

It looks like Hive.deleteBoxFromDisk doesn't works in that case because windows thinks file is currently in use as hive doesn't closes opened file on initialization failure.

This is not to say our implementation is flawless. Ofc it's still buggy as someone can call openBox without awaiting and then call box to get instance to uninitialized box object.

Maybe we can try fixing #846 in a different way instead. One way I can think of is closing box on initialization failure..

Let me know what you think about that.

/cc @Jon-Salmon, I would like to know your thoughts too

PS: sorry for comment spamming

[maRci002]

It is possible to return uninitialized box since isOpenBox is before _openingBoxes.containsKey.

https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L75-L89

Run my example code and check output.
Take 2x breakpoint
First:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L79

Second:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/backend/vm/storage_backend_vm.dart#L81

And you can see that the first breakpoint returns the box while initializing it.

[themisir]

It is possible to return uninitialized box since isOpenBox is before _openingBoxes.containsKey.

True! Missed that part, sorry. Let's merge your patch and I'll work on fixing #846.

[maRci002]

I think this can be moved before try and make is nullable
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L103

In catch block it can be closed if it's not null
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L116

I think these should be nullable
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/backend/vm/storage_backend_vm.dart#L29-L40

So close close should only close underlying file if it is not null.

This is because open can fail too and we can avoid LateInitializationError if one of them died during BackendManager.open:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L99-L100

[maRci002]

@themisir go ahead and merge. Let me know when you finished #846 fix.