Skip to content
New issue

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

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

Already on GitHub? # to your account

Updating the XMP data of same image concurrently aborts the entire process. #230

Closed
CeNiEi opened this issue Jul 25, 2024 · 7 comments
Closed
Assignees

Comments

@CeNiEi
Copy link

CeNiEi commented Jul 25, 2024

The following code, which tries to update xmp data of the SAME file concurrently, will sometimes throw SIGABRT with fatal runtime error: Rust cannot catch foreign exceptions.

#[tokio::main]
async fn main() {

    let mut handles = Vec::new();
    
    for i in 0..2 {
        let handle = spawn_blocking(move || {
            let flip = thread_rng().gen_range(1..=8);

            let (mut xmp_file, mut meta) =
                open_file("a/b/c.jpg");

            sleep(std::time::Duration::from_secs(3));
            update(&mut meta, flip);

            sleep(std::time::Duration::from_secs(3));
            write_to_file(&mut xmp_file, &meta);

        });

        handles.push(handle);
    }

    futures::stream::iter(handles)
        .buffer_unordered(4)
        .collect::<Vec<_>>()
        .await
        .into_iter()
        .collect::<Result<Vec<_>, _>>()
        .unwrap();
}

fn open_file(path: impl AsRef<std::path::Path>) -> (XmpFile, XmpMeta) {
    let path = path.as_ref();

    let mut xmp_file = XmpFile::new().unwrap();

    xmp_file
        .open_file(
            &path,
            OpenFileOptions::default()
                .only_xmp()
                .for_update()
                .use_smart_handler(),
        )
        .or_else(|_| {
            xmp_file.open_file(
                &path,
                OpenFileOptions::default()
                    .only_xmp()
                    .for_update()
                    .use_packet_scanning(),
            )
        })
        .unwrap();

    let xmp = xmp_file.xmp().unwrap_or(XmpMeta::new().unwrap());

    (xmp_file, xmp)
}

fn update(meta: &mut XmpMeta, flip: u8) {
    meta.set_property(TIFF, "Orientation", &XmpValue::new(flip.to_string()))
        .unwrap();
}

fn write_to_file(xmp_file: &mut XmpFile, meta: &XmpMeta) {
    xmp_file.put_xmp(meta).unwrap();
    xmp_file.close();
}

I get that it happens when two different instances are trying to write the data to the same file.. but it will be much better to handle the raised c++ exception and return an appropriate rust error.

@scouten-adobe
Copy link
Member

@CeNiEi thank you for the great test case. I'll look into it in next few days.

@scouten-adobe scouten-adobe self-assigned this Jul 25, 2024
scouten-adobe added a commit that referenced this issue Jul 26, 2024
@scouten-adobe
Copy link
Member

Good news: I can reliably reproduce this. See draft PR #232.

Still working out where the crash occurs.

scouten-adobe added a commit that referenced this issue Jul 26, 2024
… encountered when closing a file

Fixes crash reported in #230.
scouten-adobe added a commit that referenced this issue Jul 26, 2024
…ny error encountered when closing a file (#232)

Fixes crash reported in #230.

Thank you, @CeNiEi.
@scouten-adobe
Copy link
Member

@CeNiEi Good news: I've found a fix for the issue and it is in main branch now.

Bad news: The crate now exceeds the maximum upload size on crates.io and my attempt to fix that problem has failed.

I have to step away from the computer for a while. Will try again to publish a release when I get a moment, possibly later today; more likely sometime tomorrow.

@CeNiEi
Copy link
Author

CeNiEi commented Jul 26, 2024

@scouten-adobe Thanks a lot for the quick resolution..
I can confirm that now my code is not throwing SIGABRT and the calls the try_close method are indeed returning errors.

Although while trying to stress test this, i might have stumbled upon another issue. I tried to change xmp data of the same file from different tokio::tasks, and these were the errors which were logged.

...
XmpError { error_type: ExternalFailure, debug_message: "Host_IO::Delete, unlink failure" }
...
 XmpError { error_type: ExternalFailure, debug_message: "Host_IO::Rename, rename failure" }
...
 XmpError { error_type: NoFile, debug_message: "XMPFiles: file does not exist" }
...

I am not sure what to make of these errors, but as you can see, it seems somewhere along the line, the file itself got deleted. I checked on my disk too, the file is not there.. I am doing these tests on a JPEG which do not have a sidecar XMP file.

I'll try to replicate this on my end, and report back if I make progress...

Anyways Thanks for the help 👍

@scouten-adobe
Copy link
Member

@CeNiEi from what I can tell, the underlying C++ code is writing a new copy of the file in temporary space and then swapping out the files (deleting and renaming in quick succession?) on the close event.

If the old and new files aren't in the expected locations, then any of the errors you've described are likely. Those errors did occur in my own testing.

Now that we've made it not crash, I don't think there's a lot more we can do from the Rust side about this. If simultaneous writes to the same file are an important use case, I would encourage you to file an issue against the C++ XMP Toolkit on which this crate is based.

@CeNiEi
Copy link
Author

CeNiEi commented Jul 26, 2024

@scouten-adobe got it 👍 thanks for the help ....

@scouten-adobe
Copy link
Member

scouten-adobe commented Jul 26, 2024

@CeNiEi figured out the crate size issue just now. Just released version 1.9.0, which includes this fix. Based on your feedback, I'm closing this issue as fixed. If you find anything new that's not working, please open a new issue or reopen this one.

Thanks again for the excellent repro case. That made it a lot easier for me to fix.

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

No branches or pull requests

2 participants