Commit 36f56549 authored by Jim Nelson's avatar Jim Nelson

#1519: segault on import solved. Problem relates to #1520 (also closed in...

#1519: segault on import solved.  Problem relates to #1520 (also closed in this commit).  Completion callback 
was being re-entered due to the thumbnail import code calling spin_event_loop() (which is a legacy aspect of the 
old-style import, which was serialized in the main event thread).  Additionally, the BatchImport object was 
being derefed and freed because the Workers/BackgroundJob system does not maintain a reference to the object 
with the callbacks; this has been remedied.
parent 31149daa
......@@ -83,14 +83,17 @@ public class ImportManifest : Object {
}
public void add_result(BatchImportResult batch_result) {
bool reported = true;
switch (batch_result.result) {
case ImportResult.SUCCESS:
success.add(batch_result);
break;
case ImportResult.USER_ABORT:
if (!query_is_directory(batch_result.file))
if (batch_result.file != null && !query_is_directory(batch_result.file))
aborted.add(batch_result);
else
reported = false;
break;
case ImportResult.UNSUPPORTED_FORMAT:
......@@ -115,7 +118,8 @@ public class ImportManifest : Object {
break;
}
all.add(batch_result);
if (reported)
all.add(batch_result);
}
}
......@@ -177,6 +181,9 @@ public class BatchImport : Object {
}
~BatchImport() {
#if TRACE_DTORS
debug("DTOR: BatchImport (%s)", name);
#endif
AppWindow.get_instance().user_quit -= user_halt;
}
......@@ -192,6 +199,12 @@ public class BatchImport : Object {
cancellable.cancel();
}
private void log_status(string where) {
#if TRACE_IMPORT
debug("%s: to_perform=%d completed=%d", where, file_imports_to_perform, file_imports_completed);
#endif
}
private bool report_failures(BackgroundImportJob background_job) {
bool proceed = true;
......@@ -233,10 +246,13 @@ public class BatchImport : Object {
starting();
// fire off a background job to generate all FileToPrepare work
workers.enqueue(new WorkSniffer(jobs, on_work_sniffed_out, cancellable, on_sniffer_cancelled));
workers.enqueue(new WorkSniffer(this, jobs, on_work_sniffed_out, cancellable,
on_sniffer_cancelled));
}
private void on_work_sniffed_out(BackgroundJob j) {
assert(!completed);
WorkSniffer sniffer = (WorkSniffer) j;
if (!report_failures(sniffer) || sniffer.files_to_prepare.size == 0) {
......@@ -250,13 +266,15 @@ public class BatchImport : Object {
// to a camera without fat locking, and it's just not worth it. Serializing the imports
// also means the user sees the photos coming in in (roughly) the order they selected them
// on the screen
PrepareFilesJob prepare_files_job = new PrepareFilesJob(sniffer.files_to_prepare,
PrepareFilesJob prepare_files_job = new PrepareFilesJob(this, sniffer.files_to_prepare,
on_file_prepared, on_files_prepared, cancellable, on_file_prepare_cancelled);
workers.enqueue(prepare_files_job);
}
private void on_sniffer_cancelled(BackgroundJob j) {
assert(!completed);
WorkSniffer sniffer = (WorkSniffer) j;
report_failures(sniffer);
......@@ -264,6 +282,8 @@ public class BatchImport : Object {
}
private void on_file_prepared(BackgroundJob j, NotificationObject user) {
assert(!completed);
PreparedFile prepared_file = (PreparedFile) user;
if (TransformablePhoto.is_duplicate(prepared_file.file, prepared_file.exif_md5,
......@@ -282,19 +302,24 @@ public class BatchImport : Object {
return;
}
FileImportJob file_import_job = new FileImportJob(prepared_file, manifest.import_id,
FileImportJob file_import_job = new FileImportJob(this, prepared_file, manifest.import_id,
on_import_file_completed, cancellable, on_import_file_cancelled);
workers.enqueue(file_import_job);
}
private void on_files_prepared(BackgroundJob j) {
assert(!completed);
PrepareFilesJob prepare_files_job = (PrepareFilesJob) j;
report_failures(prepare_files_job);
// mark this job as completed and record how many file imports must finish to be complete
file_imports_to_perform = prepare_files_job.prepared_files;
assert(file_imports_to_perform >= file_imports_completed);
log_status("on_files_prepared");
// if none prepared, then none outstanding (or will become outstanding, depending on how
// the notifications are queued)
......@@ -305,11 +330,16 @@ public class BatchImport : Object {
}
private void on_file_prepare_cancelled(BackgroundJob j) {
assert(!completed);
PrepareFilesJob prepare_files_job = (PrepareFilesJob) j;
report_failures(prepare_files_job);
file_imports_to_perform = prepare_files_job.prepared_files;
assert(file_imports_to_perform >= file_imports_completed);
log_status("on_file_prepare_cancelled");
// If FileImportJobs are outstanding, need to wait for them to cancel as well ... see
// on_files_prepared for the logic of all this
......@@ -320,9 +350,15 @@ public class BatchImport : Object {
}
private void on_import_file_completed(BackgroundJob j) {
assert(!completed);
FileImportJob job = (FileImportJob) j;
file_imports_completed++;
if (file_imports_to_perform != -1)
assert(file_imports_completed <= file_imports_to_perform);
log_status("on_import_file_completed (%s)".printf(job.get_filename()));
// if success, import photo into database and in-memory data structures
LibraryPhoto photo = null;
......@@ -348,9 +384,15 @@ public class BatchImport : Object {
}
private void on_import_file_cancelled(BackgroundJob j) {
assert(!completed);
FileImportJob job = (FileImportJob) j;
file_imports_completed++;
if (file_imports_to_perform != -1)
assert(file_imports_completed <= file_imports_to_perform);
log_status("on_import_file_cancelled");
job.abort();
......@@ -385,9 +427,9 @@ private abstract class BackgroundImportJob : BackgroundJob {
public ImportResult abort_flag = ImportResult.SUCCESS;
public Gee.List<BatchImportResult> failed = new Gee.ArrayList<BatchImportResult>();
protected BackgroundImportJob(CompletionCallback callback, Cancellable cancellable,
CancellationCallback? cancellation) {
base (callback, cancellable, cancellation);
protected BackgroundImportJob(BatchImport owner, CompletionCallback callback,
Cancellable cancellable, CancellationCallback? cancellation) {
base (owner, callback, cancellable, cancellation);
}
// Subclasses should call this every iteration, and if the result is not SUCCESS, consider the
......@@ -409,11 +451,11 @@ private abstract class BackgroundImportJob : BackgroundJob {
ImportResult result) {
assert(result != ImportResult.SUCCESS);
debug("Import failure %s: %s", identifier, result.to_string());
// if fatal but the flag is not set, set it now
if (result.is_abort())
abort(result);
else
debug("Import failure %s: %s", identifier, result.to_string());
failed.add(new BatchImportResult(job, file, identifier, result));
}
......@@ -448,9 +490,9 @@ private class WorkSniffer : BackgroundImportJob {
private Gee.Iterable<BatchImportJob> jobs;
public WorkSniffer(Gee.Iterable<BatchImportJob> jobs, CompletionCallback callback,
public WorkSniffer(BatchImport owner, Gee.Iterable<BatchImportJob> jobs, CompletionCallback callback,
Cancellable cancellable, CancellationCallback cancellation) {
base (callback, cancellable, cancellation);
base (owner, callback, cancellable, cancellation);
this.jobs = jobs;
}
......@@ -562,9 +604,10 @@ private class PrepareFilesJob : BackgroundImportJob {
private int fail_every = 0;
private int skip_every = 0;
public PrepareFilesJob(Gee.List<FileToPrepare> files_to_prepare, NotificationCallback notification,
CompletionCallback callback, Cancellable cancellable, CancellationCallback cancellation) {
base (callback, cancellable, cancellation);
public PrepareFilesJob(BatchImport owner, Gee.List<FileToPrepare> files_to_prepare,
NotificationCallback notification, CompletionCallback callback, Cancellable cancellable,
CancellationCallback cancellation) {
base (owner, callback, cancellable, cancellation);
this.files_to_prepare = files_to_prepare;
this.notification = notification;
......@@ -693,9 +736,9 @@ private class FileImportJob : BackgroundJob {
private ImportID import_id;
private File final_file = null;
public FileImportJob(PreparedFile prepared_file, ImportID import_id, CompletionCallback callback,
Cancellable cancellable, CancellationCallback cancellation) {
base (callback, cancellable, cancellation);
public FileImportJob(BatchImport owner, PreparedFile prepared_file, ImportID import_id,
CompletionCallback callback, Cancellable cancellable, CancellationCallback cancellation) {
base (owner, callback, cancellable, cancellation);
this.import_id = import_id;
this.prepared_file = prepared_file;
......
......@@ -4,14 +4,13 @@
* See the COPYING file in this distribution.
*/
public class PixbufCache {
public class PixbufCache : Object {
public enum PhotoType {
REGULAR,
ORIGINAL
}
private abstract class FetchJob : BackgroundJob {
public PixbufCache owner;
public BackgroundJob.JobPriority priority;
public TransformablePhoto photo;
public Scaling scaling;
......@@ -20,11 +19,8 @@ public class PixbufCache {
public FetchJob(PixbufCache owner, BackgroundJob.JobPriority priority, TransformablePhoto photo,
Scaling scaling, CompletionCallback callback, Cancellable cancellable) {
base(callback, cancellable);
base(owner, callback, cancellable);
// maintain the owner ref because Workers do not, and if the PixbufCache is derefed
// before a job completes, an assertion fires
this.owner = owner;
this.priority = priority;
this.photo = photo;
this.scaling = scaling;
......
......@@ -84,7 +84,7 @@ public class ThumbnailCache : Object {
public AsyncFetchJob(ThumbnailCache cache, PhotoID photo_id, Gdk.Pixbuf? prefetched,
Dimensions dim, Gdk.InterpType interp, AsyncFetchCallback callback, Cancellable? cancellable) {
base(async_fetch_completion_callback, cancellable);
base(cache, async_fetch_completion_callback, cancellable);
this.cache = cache;
this.photo_id = photo_id;
......@@ -162,35 +162,23 @@ public class ThumbnailCache : Object {
public static void import_from_source(PhotoID photo_id, PhotoSource source, bool force = false)
throws Error {
big._import_from_source(photo_id, source, force);
spin_event_loop();
medium._import_from_source(photo_id, source, force);
spin_event_loop();
}
public static void import_thumbnails(PhotoID photo_id, Thumbnails thumbnails, bool force = false)
throws Error {
big._import_thumbnail(photo_id, thumbnails.get(Size.BIG), force);
spin_event_loop();
medium._import_thumbnail(photo_id, thumbnails.get(Size.MEDIUM), force);
spin_event_loop();
}
public static void duplicate(PhotoID src_id, PhotoID dest_id) {
big._duplicate(src_id, dest_id);
spin_event_loop();
medium._duplicate(src_id, dest_id);
spin_event_loop();
}
public static void remove(PhotoID photo_id) {
big._remove(photo_id);
spin_event_loop();
medium._remove(photo_id);
spin_event_loop();
}
private static ThumbnailCache get_best_cache(int scale) {
......
......@@ -19,7 +19,7 @@ public delegate void CancellationCallback(BackgroundJob job);
//
// Note that there does not seem to be any guarantees of order in the Idle queue documentation,
// and this it's possible (and, depending on assigned priorities, likely) that notifications could
// arrive in different orders, and even after the CompletionCallback. Thus, no guarantees of
// arrive in different orders, and even after the CompletionCallback. Thus, no guarantee of
// ordering is made here.
//
// NOTE: Would like Value to be nullable, but can't due to this bug:
......@@ -39,7 +39,15 @@ public delegate void NotificationCallback(BackgroundJob job, NotificationObject
// This abstract class represents a unit of work that can be executed within a background thread's
// context. If specified, the job may be cancellable (which can be checked by execute() and the
// worker thread prior to calling execute()). The BackgroundJob may also specify a
// CompletionCallback to be executed within Gtk's event loop.
// CompletionCallback and/or a CancellationCallback to be executed within Gtk's event loop.
// A BackgroundJob may also emit NotificationCallbacks, all of which are also executed within
// Gtk's event loop.
//
// The BackgroundJob may be constructed with a reference to its "owner". This is not used directly
// by BackgroundJob or Worker, but merely exists to hold a reference to the Object that is receiving
// the various callbacks from BackgroundJob. Without this, it's possible for the object creating
// BackgroundJobs to be freed before all the callbacks have been received, or even during a callback,
// which is an unstable situation.
public abstract class BackgroundJob {
public enum JobPriority {
HIGHEST = 100,
......@@ -69,6 +77,7 @@ public abstract class BackgroundJob {
private static Gee.ArrayList<NotificationJob> notify_queue = new Gee.ArrayList<NotificationJob>();
private Object owner;
private CompletionCallback callback;
private Cancellable cancellable;
private CancellationCallback cancellation;
......@@ -82,8 +91,9 @@ public abstract class BackgroundJob {
private int completion_priority = Priority.HIGH;
private int notification_priority = Priority.DEFAULT_IDLE;
public BackgroundJob(CompletionCallback? callback = null, Cancellable? cancellable = null,
CancellationCallback? cancellation = null) {
public BackgroundJob(Object? owner = null, CompletionCallback? callback = null,
Cancellable? cancellable = null, CancellationCallback? cancellation = null) {
this.owner = owner;
this.callback = callback;
this.cancellable = cancellable;
this.cancellation = cancellation;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment