(MINOR) Add new API function XmpFile::try_close which will report a…

…ny error encountered when closing a file (#232)

Fixes crash reported in #230.

Thank you, @CeNiEi.

Cargo.toml

@@ -42,5 +42,9 @@ fs_extra = "1.3"
 
 [dev-dependencies]
 anyhow = "1.0.67"
+futures = "0.3.30"
 pretty_assertions = "1.4.0"
+rand = "0.8.5"
 tempfile = "3.2"
+tokio = { version = "1.39", features = ["full"] }
+tokio-macros = "2.4"

src/ffi.cpp

@@ -221,12 +221,21 @@ extern "C" {
         #endif
     }
 
-    void CXmpFileClose(CXmpFile* f) {
+    void CXmpFileClose(CXmpFile* f,
+                       CXmpError* outError) {
         #ifndef NOOP_FFI
             // TO DO: Bridge closeFlags parameter.
             // For my purposes at the moment,
             // default value (0) always suffices.
-            f->f.CloseFile();
+            try {
+                f->f.CloseFile();
+            }
+            catch (XMP_Error& e) {
+                copyErrorForResult(e, outError);
+            }
+            catch (...) {
+                signalUnknownError(outError);
+            }
         #endif
     }
 

src/ffi.rs

@@ -148,7 +148,7 @@ extern "C" {
         flags: u32,
     );
 
-    pub(crate) fn CXmpFileClose(file: *mut CXmpFile);
+    pub(crate) fn CXmpFileClose(file: *mut CXmpFile, out_error: *mut CXmpError);
     pub(crate) fn CXmpFileGetXmp(file: *mut CXmpFile) -> *mut CXmpMeta;
 
     pub(crate) fn CXmpFilePutXmp(

src/tests/issues/issue_230.rs

@@ -0,0 +1,148 @@
+// Copyright 2024 Adobe. All rights reserved.
+// This file is licensed to you under the Apache License,
+// Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
+// or the MIT license (http://opensource.org/licenses/MIT),
+// at your option.
+
+// Unless required by applicable law or agreed to in writing,
+// this software is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR REPRESENTATIONS OF ANY KIND, either express or
+// implied. See the LICENSE-MIT and LICENSE-APACHE files for the
+// specific language governing permissions and limitations under
+// each license.
+
+// Test case for https://github.com/adobe/xmp-toolkit-rs/issues/230:
+// Updating the XMP data of same image concurrently aborts the entire process.
+
+#![cfg(not(windows))]
+// Windows version of C++ XMP Toolkit reports errors with simultaneous file
+// access, so this test is disabled on Windows.
+
+use std::thread::sleep;
+
+use futures::stream::StreamExt;
+use rand::{thread_rng, Rng};
+use tempfile::tempdir;
+use tokio::task::spawn_blocking;
+use tokio_macros::test;
+
+use crate::{
+    tests::fixtures::temp_copy_of_fixture, xmp_ns::TIFF, OpenFileOptions, XmpFile, XmpMeta,
+    XmpValue,
+};
+
+#[test(flavor = "multi_thread")]
+async fn original_bug() {
+    let tempdir: tempfile::TempDir = tempdir().unwrap();
+    let image2 = temp_copy_of_fixture(tempdir.path(), "image2.jpg");
+
+    let mut handles = Vec::new();
+
+    for _ in 0..2 {
+        let image2 = image2.clone();
+
+        let handle = spawn_blocking(move || {
+            let flip = thread_rng().gen_range(1..=8);
+
+            let (mut xmp_file, mut meta) = open_file(&image2);
+
+            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();
+}
+
+#[test(flavor = "multi_thread")]
+async fn new_api_try_close() {
+    let tempdir: tempfile::TempDir = tempdir().unwrap();
+    let image2 = temp_copy_of_fixture(tempdir.path(), "image2.jpg");
+
+    let mut handles = Vec::new();
+
+    for _ in 0..2 {
+        let image2 = image2.clone();
+
+        let handle = spawn_blocking(move || {
+            let flip = thread_rng().gen_range(1..=8);
+
+            let (mut xmp_file, mut meta) = open_file(&image2);
+
+            sleep(std::time::Duration::from_secs(3));
+            update(&mut meta, flip);
+
+            sleep(std::time::Duration::from_secs(3));
+            write_to_file_try_close(&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();
+}
+
+fn write_to_file_try_close(xmp_file: &mut XmpFile, meta: &XmpMeta) {
+    xmp_file.put_xmp(meta).unwrap();
+    let _ = xmp_file.try_close();
+    // This is a race condition: We can't predict which thread
+    // will encounter the error on close, so we ignore it.
+    // The primary concern here is that we no longer abort the process when that
+    // error is encountered.
+}

src/tests/issues/mod.rs

@@ -0,0 +1,16 @@
+// Copyright 2024 Adobe. All rights reserved.
+// This file is licensed to you under the Apache License,
+// Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
+// or the MIT license (http://opensource.org/licenses/MIT),
+// at your option.
+
+// Unless required by applicable law or agreed to in writing,
+// this software is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR REPRESENTATIONS OF ANY KIND, either express or
+// implied. See the LICENSE-MIT and LICENSE-APACHE files for the
+// specific language governing permissions and limitations under
+// each license.
+
+// Test cases for specific GitHub issues will be added here.
+
+mod issue_230;

src/tests/mod.rs

@@ -19,6 +19,7 @@
 #![allow(clippy::unwrap_used)]
 
 mod fixtures;
+mod issues;
 mod xmp_core_coverage;
 mod xmp_date_time;
 #[cfg(feature = "chrono")]

src/xmp_file.rs

@@ -170,8 +170,41 @@ impl XmpFile {
     /// [`XmpFile::close`] is called. The disk file is only updated once,
     /// when [`XmpFile::close`] is called, regardless of how many calls are
     /// made to [`XmpFile::put_xmp`].
+    ///
+    /// IMPORTANT: The original design of this API did not include any
+    /// error reporting, so this function ignores any error that may be
+    /// reported by the C++ XMP Toolkit. Use [`XmpFile::try_close`] to
+    /// receive any error reporting.
     pub fn close(&mut self) {
-        unsafe { ffi::CXmpFileClose(self.f) };
+        let _ = self.try_close();
+        // Ignore error for backward compatibility.
+        // If you want to see the error, use try_close().
+    }
+
+    /// Explicitly closes an opened file.
+    ///
+    /// Performs any necessary output to the file and closes it. Files that are
+    /// opened for update are written to only when closing.
+    ///
+    /// If the file is opened for read-only access (passing
+    /// [`OpenFileOptions::for_read`]), the disk file is closed
+    /// immediately after reading the data from it; the `XMPFile`
+    /// struct, however, remains in the open state. You must call
+    /// [`XmpFile::close`] when finished using it. Other methods, such as
+    /// [`XmpFile::xmp`], can only be used between the
+    /// [`XmpFile::open_file`] and [`XmpFile::close`] calls. The `XMPFile`
+    /// destructor does not call [`XmpFile::close`]; if the struct is
+    /// dropped without closing, any pending updates are lost.
+    ///
+    /// If the file is opened for update (passing
+    /// [`OpenFileOptions::for_update`]), the disk file remains open until
+    /// [`XmpFile::close`] is called. The disk file is only updated once,
+    /// when [`XmpFile::close`] is called, regardless of how many calls are
+    /// made to [`XmpFile::put_xmp`].
+    pub fn try_close(&mut self) -> XmpResult<()> {
+        let mut err = ffi::CXmpError::default();
+        unsafe { ffi::CXmpFileClose(self.f, &mut err) };
+        XmpError::raise_from_c(&err)
     }
 }