Thumbnailer

Merge lp:~fboucault/thumbnailer/save_failures into lp:thumbnailer

  1. save_failures
  2. Merge into trunk
Proposed by Florian Boucault on 2015-01-15
Status: Merged
Approved by: Jussi Pakkanen on 2015-01-22
Approved revision: 121
Merged at revision: 117
Proposed branch: lp:~fboucault/thumbnailer/save_failures
Merge into: lp:thumbnailer
Diff against target: 157 lines (+73/-4)
3 files modified
include/internal/thumbnailcache.h (+3/-0)
src/thumbnailcache.cpp (+55/-0)
src/thumbnailer.cpp (+15/-4)
To merge this branch: bzr merge lp:~fboucault/thumbnailer/save_failures
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-01-21
Jussi Pakkanen (community) 2015-01-15 Approve on 2015-01-21
Review via email: mp+246547@code.launchpad.net

Commit Message

Keeps track of failed thumbnails and do not try to regenerate them.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Jussi Pakkanen (jpakkane) wrote :

In mark failure you have this:

  FILE *f = fopen(fail_name.c_str(), "wb+");
  fclose(f);

The documentation of fclose says that if f is invalid, the behaviour is undefined. This can happen if fopen fails. Please wrap the fclose in an if(f). If the creation fails we can't throw an exception as one is already in flight. So just write strerror to stderr instead.

review: Needs Fixing
Jussi Pakkanen (jpakkane) wrote :

Also, catch the exception by const ref instead of plain ref and define mark_failure as noexcept.

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~fboucault/thumbnailer/save_failures updated on 2015-01-20
118. By Florian Boucault on 2015-01-20

Catch with const exception

119. By Florian Boucault on 2015-01-20

Mark ThumbnailCache::has_failure as noexcept

120. By Florian Boucault on 2015-01-20

Mark ThumbnailCache::has_failure as not returning any value.

121. By Florian Boucault on 2015-01-20

mark_failure: handle error case better

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Jussi Pakkanen (jpakkane) wrote :

Looking great. Thanks.

review: Approve
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/internal/thumbnailcache.h'
2--- include/internal/thumbnailcache.h 2014-09-15 08:07:44 +0000
3+++ include/internal/thumbnailcache.h 2015-01-20 15:35:31 +0000
4@@ -36,6 +36,9 @@
5
6 std::string get_if_exists(const std::string &abs_path, ThumbnailSize desired_size) const;
7 std::string get_cache_file_name(const std::string &as_path, ThumbnailSize desired_size) const;
8+ std::string get_fail_file_name(const std::string &as_path) const;
9+ void mark_failure(const std::string &abs_path) noexcept;
10+ bool has_failure(const std::string &abs_path) const;
11 void clear();
12 void prune();
13
14
15=== modified file 'src/thumbnailcache.cpp'
16--- src/thumbnailcache.cpp 2015-01-05 08:47:31 +0000
17+++ src/thumbnailcache.cpp 2015-01-20 15:35:31 +0000
18@@ -132,6 +132,7 @@
19
20 string md5(const string &str) const;
21 string get_cache_file_name(const std::string &original, ThumbnailSize desired) const;
22+ string get_fail_file_name(const std::string &original) const;
23 void clear();
24 void delete_from_cache(const std::string &abs_path);
25 void prune();
26@@ -142,6 +143,7 @@
27 string largedir;
28 string xlargedir;
29 string originaldir;
30+ string faildir;
31 static const size_t MAX_FILES = 200;
32
33 };
34@@ -151,6 +153,7 @@
35 cleardir(largedir);
36 cleardir(xlargedir);
37 cleardir(originaldir);
38+ cleardir(faildir);
39 }
40
41
42@@ -159,6 +162,7 @@
43 prune_dir(largedir, MAX_FILES);
44 prune_dir(xlargedir, MAX_FILES);
45 prune_dir(originaldir, MAX_FILES);
46+ prune_dir(faildir, MAX_FILES);
47 }
48
49 void ThumbnailCachePrivate::delete_from_cache(const std::string &abs_path) {
50@@ -166,6 +170,7 @@
51 unlink(get_cache_file_name(abs_path, TN_SIZE_LARGE).c_str());
52 unlink(get_cache_file_name(abs_path, TN_SIZE_XLARGE).c_str());
53 unlink(get_cache_file_name(abs_path, TN_SIZE_ORIGINAL).c_str());
54+ unlink(get_fail_file_name(abs_path).c_str());
55 }
56
57 static string makedir(const string &base, const string &subdir) {
58@@ -226,6 +231,7 @@
59 largedir = makedir(tndir, "large");
60 xlargedir = makedir(tndir, "xlarge");
61 originaldir = makedir(tndir, "original");
62+ faildir = makedir(tndir, "fail");
63 }
64
65 string ThumbnailCachePrivate::md5(const string &str) const {
66@@ -261,6 +267,14 @@
67 return path;
68 }
69
70+string ThumbnailCachePrivate::get_fail_file_name(const std::string & abs_original) const {
71+ assert(!abs_original.empty());
72+ assert(abs_original[0] == '/');
73+ string path = faildir;
74+ path += "/" + md5("file://" + abs_original) + ".png";
75+ return path;
76+}
77+
78 ThumbnailCache::ThumbnailCache() : p(new ThumbnailCachePrivate()) {
79 // This should never happen. If new fails, the above initializer
80 // should throw an exception. But nonetheless this has been
81@@ -307,6 +321,47 @@
82 return p->get_cache_file_name(abs_path, desired);
83 }
84
85+std::string ThumbnailCache::get_fail_file_name(const std::string &abs_path) const {
86+ return p->get_fail_file_name(abs_path);
87+}
88+
89+void ThumbnailCache::mark_failure(const std::string &abs_path) noexcept {
90+ assert(abs_path[0] == '/');
91+
92+ string fail_name = p->get_fail_file_name(abs_path);
93+ FILE *f = fopen(fail_name.c_str(), "wb+");
94+ if (f) {
95+ fclose(f);
96+ } else {
97+ fprintf(stderr, "Could not mark thumbnailing failure: %s\n", strerror(errno));
98+ }
99+}
100+
101+bool ThumbnailCache::has_failure(const std::string &abs_path) const {
102+ assert(abs_path[0] == '/');
103+
104+ string fail_name = p->get_fail_file_name(abs_path);
105+ FILE *f = fopen(fail_name.c_str(), "r");
106+ bool fail_exists = false;
107+ if(f) {
108+ fail_exists = true;
109+ fclose(f);
110+ }
111+
112+ // Has it been changed since the thumbnail was taken?
113+ if(fail_exists) {
114+ struct stat original_stat, fail_stat;
115+ stat(abs_path.c_str(), &original_stat);
116+ stat(fail_name.c_str(), &fail_stat);
117+ if(original_stat.st_mtime > fail_stat.st_mtime) {
118+ p->delete_from_cache(abs_path);
119+ fail_exists = false;
120+ }
121+ }
122+
123+ return fail_exists;
124+}
125+
126 void ThumbnailCache::clear() {
127 p->clear();
128 }
129
130=== modified file 'src/thumbnailer.cpp'
131--- src/thumbnailer.cpp 2015-01-05 08:47:31 +0000
132+++ src/thumbnailer.cpp 2015-01-20 15:35:31 +0000
133@@ -229,10 +229,21 @@
134 std::string estimate = p->cache.get_if_exists(abspath, desired_size);
135 if(!estimate.empty())
136 return estimate;
137- string generated = p->create_thumbnail(abspath, desired_size, policy);
138- if(generated == abspath) {
139- return abspath;
140- }
141+
142+ if (p->cache.has_failure(abspath)) {
143+ throw runtime_error("Thumbnail generation failed previously, not trying again.");
144+ }
145+
146+ try {
147+ string generated = p->create_thumbnail(abspath, desired_size, policy);
148+ if(generated == abspath) {
149+ return abspath;
150+ }
151+ } catch(const std::runtime_error &e) {
152+ p->cache.mark_failure(abspath);
153+ throw;
154+ }
155+
156 return p->cache.get_if_exists(abspath, desired_size);
157 }
158

Subscribers

People subscribed via source and target branches

to all changes: