From 43af50bd8aea764eb526b775a63b1df0488eda16 Mon Sep 17 00:00:00 2001 From: John Ralls Date: Sat, 14 Jul 2018 17:09:22 -0700 Subject: [PATCH] Bug 796759 - --add-price-quotes leaves a lock on the file. First, save isn't necessary if the book is dirty, so don't... but that means that the book has to be marked dirty after a session swap. No more laziness. Second, regardless of the outcome of inner_main_add_price_quotes the session must be destroyed to remove the lock. A couple of cleanups in QofSessionImpl::save as well: Rewrote the descriptive comment to reflect how it really works when the backend has gotten disconnected and removed the superfluous qof_book_set_backend with the backend that we'd *just gotten from the book*. --- gnucash/gnome-utils/gnc-file.c | 6 +----- gnucash/gnucash-bin.c | 9 +++++++-- .../dbi/test/test-backend-dbi-basic.cpp | 4 ++++ libgnucash/engine/qofsession.cpp | 20 +++++++++---------- libgnucash/engine/test/test-qofsession.cpp | 1 + 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/gnucash/gnome-utils/gnc-file.c b/gnucash/gnome-utils/gnc-file.c index 0f19e5f999..72dddbef39 100644 --- a/gnucash/gnome-utils/gnc-file.c +++ b/gnucash/gnome-utils/gnc-file.c @@ -1536,11 +1536,7 @@ gnc_file_do_save_as (GtkWindow *parent, const char* filename) /* if we got to here, then we've successfully gotten a new session */ /* close up the old file session (if any) */ qof_session_swap_data (session, new_session); - - /* XXX At this point, we should really mark the data in the new session - * as being 'dirty', since we haven't saved it at all under the new - * session. But I'm lazy... - */ + qof_book_mark_session_dirty (qof_session_get_book (new_session)); qof_event_resume(); diff --git a/gnucash/gnucash-bin.c b/gnucash/gnucash-bin.c index 441fa2a31c..2514e4945c 100644 --- a/gnucash/gnucash-bin.c +++ b/gnucash/gnucash-bin.c @@ -585,8 +585,13 @@ inner_main_add_price_quotes(void *closure, int argc, char **argv) gnc_shutdown(0); return; fail: - if (session && qof_session_get_error(session) != ERR_BACKEND_NO_ERR) - g_warning("Session Error: %s", qof_session_get_error_message(session)); + if (session) + { + if (qof_session_get_error(session) != ERR_BACKEND_NO_ERR) + g_warning("Session Error: %s", + qof_session_get_error_message(session)); + qof_session_destroy(session); + } qof_event_resume(); gnc_shutdown(1); } diff --git a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp index 7fb08a9d1b..1447f396ce 100644 --- a/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp +++ b/libgnucash/backend/dbi/test/test-backend-dbi-basic.cpp @@ -401,6 +401,7 @@ test_dbi_store_and_reload (Fixture* fixture, gconstpointer pData) g_assert (session_2 != NULL); g_assert_cmpint (qof_session_get_error (session_2), == , ERR_BACKEND_NO_ERR); qof_session_swap_data (fixture->session, session_2); + qof_book_mark_session_dirty (qof_session_get_book (session_2)); qof_session_save (session_2, NULL); g_assert (session_2 != NULL); g_assert_cmpint (qof_session_get_error (session_2), == , ERR_BACKEND_NO_ERR); @@ -459,6 +460,7 @@ test_dbi_safe_save (Fixture* fixture, gconstpointer pData) goto cleanup; } qof_session_swap_data (fixture->session, session_1); + qof_book_mark_session_dirty (qof_session_get_book (session_1)); qof_session_save (session_1, NULL); /* Do a safe save */ qof_session_safe_save (session_1, NULL); @@ -532,6 +534,7 @@ test_dbi_version_control (Fixture* fixture, gconstpointer pData) goto cleanup; } qof_session_swap_data (fixture->session, sess); + qof_book_mark_session_dirty (qof_session_get_book (sess)); qof_session_save (sess, NULL); sql_be = reinterpret_cast(qof_session_get_backend (sess)); book = qof_session_get_book (sess); @@ -587,6 +590,7 @@ test_dbi_business_store_and_reload (Fixture* fixture, gconstpointer pData) session_2 = qof_session_new (); qof_session_begin (session_2, url, FALSE, TRUE, TRUE); qof_session_swap_data (fixture->session, session_2); + qof_book_mark_session_dirty (qof_session_get_book (session_2)); qof_session_save (session_2, NULL); // Reload the session data diff --git a/libgnucash/engine/qofsession.cpp b/libgnucash/engine/qofsession.cpp index 0c57736517..2898d5d16a 100644 --- a/libgnucash/engine/qofsession.cpp +++ b/libgnucash/engine/qofsession.cpp @@ -451,23 +451,21 @@ QofSessionImpl::is_saving () const noexcept void QofSessionImpl::save (QofPercentageFunc percentage_func) noexcept { + if (!qof_book_session_not_saved (m_book)) //Clean book, nothing to do. + return; m_saving = true; ENTER ("sess=%p book_id=%s", this, m_book_id.c_str ()); - /* If there is a backend, and the backend is reachable - * (i.e. we can communicate with it), then synchronize with - * the backend. If we cannot contact the backend (e.g. - * because we've gone offline, the network has crashed, etc.) - * then give the user the option to save to the local disk. - * - * hack alert -- FIXME -- XXX the code below no longer - * does what the words above say. This needs fixing. - */ + /* If there is a backend, the book is dirty, and the backend is reachable + * (i.e. we can communicate with it), then synchronize with the backend. If + * we cannot contact the backend (e.g. because we've gone offline, the + * network has crashed, etc.) then raise an error so that the controlling + * dialog can offer the user a chance to save in a different way. + */ auto backend = qof_book_get_backend (m_book); if (backend) { - /* if invoked as SaveAs(), then backend not yet set */ - qof_book_set_backend (m_book, backend); + backend->set_percentage(percentage_func); backend->sync(m_book); auto err = backend->get_error(); diff --git a/libgnucash/engine/test/test-qofsession.cpp b/libgnucash/engine/test/test-qofsession.cpp index 38a7cc6f10..595eb62910 100644 --- a/libgnucash/engine/test/test-qofsession.cpp +++ b/libgnucash/engine/test/test-qofsession.cpp @@ -192,6 +192,7 @@ TEST (QofSessionTest, save) qof_backend_register_provider (get_provider ()); QofSession s; s.begin ("book1", false, false, false); + qof_book_mark_session_dirty (s.get_book ()); s.save (nullptr); EXPECT_EQ (sync_called, true); qof_backend_unregister_all_providers ();