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 incorrect JNI signature for PatchAction.FlagConflict constructor #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

turix2
Copy link

@turix2 turix2 commented Jun 30, 2024

When I do something like the following for an Automerge Document object called _doc with SyncState called _syncState:

    PatchLog patchLog = new PatchLog();
    _doc.receiveSyncMessage(_syncState, patchLog, msg);
    for (Patch p : _doc.makePatches(patchLog)) {
           // determine what types of patch we had here...
    }

I get the following panic if there are any PatchAction.FlagConflict actions among the patches:

thread '<unnamed>' panicked at src/patches.rs:152:56:
called `Result::unwrap()` on an `Err` value: JavaException
stack backtrace:
   0:        0x11759be7b - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h958f6e6821e9b0fb
   1:        0x1175b8073 - core::fmt::write::hb5e3c29742bab55e
   2:        0x11759a65e - std::io::Write::write_fmt::h3f38404afa442946
   3:        0x11759bc59 - std::sys_common::backtrace::print::h1ce04ba6121a0174
   4:        0x11759ce95 - std::panicking::default_hook::{{closure}}::hf2e5fe71523bcace
   5:        0x11759cc14 - std::panicking::default_hook::h4f8cdc98d2dcc8c0
   6:        0x11759d2f3 - std::panicking::rust_panic_with_hook::h28420d44f043d3a5
   7:        0x11759d20e - std::panicking::begin_panic_handler::{{closure}}::h6f886c0e89185cdc
   8:        0x11759c359 - std::sys_common::backtrace::__rust_end_short_backtrace::h7ca2b2ff22d46410
   9:        0x11759cf62 - _rust_begin_unwind
  10:        0x1175cc775 - core::panicking::panic_fmt::h52dad7a658d9bf41
  11:        0x1175ccc15 - core::result::unwrap_failed::h84bbbea2e5d8da9f
  12:        0x117453203 - automerge_jni::patches::to_patch_arraylist::h1da89d0366550522
  13:        0x117468237 - _Java_org_automerge_AutomergeSys_makePatches
fatal runtime error: failed to initiate panic, error 5
Abort trap: 6

I believe this PR fixes that problem. The JNI signature to call the Java FlagConflict constructor from Rust was incorrect: there should only be 1 argument (the Prop), not 2.

@alexjg
Copy link
Collaborator

alexjg commented Jun 30, 2024

Thanks for catching this, could you also add a test which reproduces the problem so it doesn't occur again

@turix2
Copy link
Author

turix2 commented Jun 30, 2024

Thanks for catching this, could you also add a test which reproduces the problem so it doesn't occur again

Unfortunately, I don't have the build environment set up, and it's too much work for me to do so right now on my machine. Sorry!

But the test wouldn't be that complicated. I think you just need to set up a merge conflict and then ask for the patches. Something like the following (which I just wrote here and have not even tried to compile) should do it:

    Document doc1 = new Document();
    SyncState ss1 = new SyncState();

    Document doc2 = new Document();
    SyncState ss2 = new SyncState();

    try (Transaction tx1 = doc1.startTransaction()) {
        tx1.set(ObjectId.ROOT, "key", ObjectType.MAP);
        tx1.commit();
    }
    try (Transaction tx2 = doc2.startTransaction()) {
        tx2.set(ObjectId.ROOT, "key", ObjectType.MAP);
        tx2.commit();
    }
    
    byte[] m1to2, m2to1;
    PatchLog pl;
    List<Patch> patches;
    while (true) {
        m1to2 = doc1.generateSyncMessage(ss1).orElse(new byte[0]);

        pl = new PatchLog();
        doc2.receiveSyncMessage(ss2, pl, m1to2);
        patches = doc2.makePatches(pl);    // may panic, but shouldn't after fix

        m2to1 = doc2.generateSyncMessage(ss2).orElse(new byte[0]);

        pl = new PatchLog();
        doc1.receiveSyncMessage(ss1, pl, m2to1);
        patches = doc1.makePatches(pl);    // may panic, but shouldn't after fix

        if (m1to2.length == 0  &&  m2to1.length == 0)
            break;
    }

It may not panic on the first set of messages exchanged in the "handshake" between 1 and 2, but I think it will before they converge.

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.

2 participants