Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: This should not happen. Please open an issue on GitHub. #914

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

maRci002
Copy link
Contributor

Hello, this PR prevents the occurence of the well known HiveError: This should not happen. Please open an issue on GitHub. error moreover it fixes these boxes if the box becomes unrecoverable.

This PR has two commits:

  1. prevents this error from happening (+ tests)
  2. recovers box if error already happened (+ tests)

When / How does this error happen?

Short answer

The problem happens when Hive tries to open a corrupted box which is recoverable however unfortunately the recover process is working incorrectly and if Hive tries to put something in the "recovered" box then the box becomes unrecoverable but it can be used for write / read until the box is closed and whenever Hive wants open this unrecoverable box the above error happens.

Long answer + example code

Demonstration example:

import 'dart:io';
import 'dart:typed_data';

import 'package:hive/hive.dart';

const boxName = 'test_box';
const frame1 = 'frame1';
const frame2 = 'frame2';

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

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

  Hive.init(dbDirectory.path);

  // 1) open db / write 2 frames / close / dump
  var box = await Hive.openBox(boxName);
  // remain simple: key / value are same so it is easier to examine dumped raw bytes
  await box.put(frame1, frame1);
  await box.put(frame2, frame2);
  await box.close();
  await dumpDatabase(dbFile, 1);

  // 2) corrupting database / dump
  await corruptingDatabase(dbFile);
  await dumpDatabase(dbFile, 2);

  // 3) open db / dump
  box = await Hive.openBox(boxName);
  await dumpDatabase(dbFile, 3);

  // 4) write 1 frame / close / dump
  await box.put(frame2, frame2);
  await box.close();
  await dumpDatabase(dbFile, 4);

  // 5) open database / close / dump
  box = await Hive.openBox(boxName);
  await box.close();
  await dumpDatabase(dbFile, 5);
}

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

Future<void> dumpDatabase(File dbFile, int title) async {
  Uint8List? bytes;

  if (await dbFile.exists()) {
    bytes = await dbFile.readAsBytes();
  }

  print('raw bytes ( $title ): $bytes');
}

Future<void> corruptingDatabase(File file) async {
  // mess up 2. Frame, for example mess up CRC check or data itself
  // I'm going to mess up data itself
  final len = await file.length();

  final raf = await file.open(mode: FileMode.writeOnlyAppend);
  // last byte of payload which is currently 50, lets override it to 51, so CRC check should fail next time (2. frame become corrupted)
  await raf.setPosition(len - 5);
  await raf.writeByte(51);
  await raf.close();
}

Let me break the output of the example:
1) open db / write 2 frames / close / dump

raw bytes ( 1 ): [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 49, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 49, 13, 235, 12, 102, 27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 50, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 50, 71, 104, 155, 136]

These raw bytes contain two frames:

  1. [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 49, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 49, 13, 235, 12, 102]
    • 27, 0, 0, 0, means frame's length is 27
    • 1, means FrameKeyType is asciiStringT
    • 6, means the key frame1 is 6 length
    • 102, 114, 97, 109, 101, 49, means the key frame1 itself represented as UTF-16 code
    • 4, means FrameValueType is stringT
    • 6, 0, 0, 0, means the value frame1 is 6 length
    • 102, 114, 97, 109, 101, 49, means the value frame1 itself represented as UTF-16 code
    • 13, 235, 12, 102 CRC check
  2. [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 50, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 50, 71, 104, 155, 136]
    • this is basically the same however this time the key and value are frame2 so the 102, 114, 97, 109, 101, 49, parts become 102, 114, 97, 109, 101, 50,

2) corrupting database / dump

raw bytes ( 2 ): [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 49, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 49, 13, 235, 12, 102, 27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 50, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 51, 71, 104, 155, 136]

As you can see 102, 114, 97, 109, 101, 50, part is changed to 102, 114, 97, 109, 101, 51, which would mean the value is frame3 instead of frame2 however CRC check will know that this frame is corrupted

3) open db / dump

raw bytes ( 3 ): [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 49, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 49, 13, 235, 12, 102]

It seems that Hive has successfully recovered the box.

4) write 1 frame / close / dump

raw bytes ( 4 ): [27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 49, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 49, 13, 235, 12, 102, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 27, 0, 0, 0, 1, 6, 102, 114, 97, 109, 101, 50, 4, 6, 0, 0, 0, 102, 114, 97, 109, 101, 50, 71, 104, 155, 136]

0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, this part is the main problem and according to current Hive implementation this results in an unrecoverable box.

Note: here we can still write as many frames as we want and read them since the keyStore caches them.

5) open database / close / dump
HiveError: This should not happen. Please open an issue on GitHub.

What happened?
The main problem is when Hive tries to recover the recoverable box it does the following:

    if (recoveryOffset != -1) {
      if (_crashRecovery) {
        print('Recovering corrupted box.');
        await writeRaf.truncate(recoveryOffset);
        writeOffset = recoveryOffset;
      } else {
        throw HiveError('Wrong checksum in hive file. Box may be corrupted.');
      }
    }

The above snippet forgets to set writeRaf's position to recoveryOffset. The writeOffset is set appropriately which basically helps frames identify their start offset which is useful for instance: sorting frames so deleted ones can be determined / lazy frames can be read from filesystem.

Closes #263

Copy link
Contributor

@themisir themisir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you sooo much for this PR!

hive/lib/src/binary/binary_reader_impl.dart Show resolved Hide resolved
@unacorbatanegra
Copy link

Awesome work, thanks!

@juandiago
Copy link

just....thanks!! great work!

@jamesdixon
Copy link

Excellent work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhandled Exception: HiveError: This should not happen. Please open an issue on GitHub.
5 participants