Created attachment 187140 [details] [review]
Patch

I'm changing the format used to serialize/deserialize cache entries. So trying to use this code with the old format cache file will result in crashes. Should apps take care of this or is it better to detect that situation in libsoup?

Please take care of it in libsoup. At worst just remove all cache data when the old format is detected and start from scratch again, I suppose a migration is overkill.

Created attachment 187177 [details] [review]
Patch v2

It now wipes out the files in cache that use the old format.

Comment on attachment 187177 [details] [review]
Patch v2

So, if you install git bz (git://git.fishsoup.net/git-bz), then you can write the patch, commit it to your local tree with a proper commit message, and then do "git bz attach 649309 HEAD" to attach the patch here (in git "mailbox" format, including the commit message and everything), "git bz attach -e HEAD" to attach an updated version and mark the old one obsolete, and "git bz push" to do a "git push" and also close the bug all at once.

(Well, making "git bz attach" know what server/product to use will require using git config first, but it will explain what commands you need to type the first time you try it.)

And then since it's so easy to deal with multiple patches, you can split your commit into separate pieces for each separate change :-) (and do "git bz attach 649309 origin/master.." to attach everything you've committed locally that's different from upstream).


Also, I run all the tests in tests/ under valgrind at least once a release cycle, so if you add a cache-test there, then I'll notice leaks myself at some point.


Anyway...

>-	entry = g_slice_new0 (SoupCacheEntry);
>+	entry = g_malloc0 (sizeof(SoupCacheEntry));

>-	g_slice_free (SoupCacheEntry, entry);
>+	g_free (entry);

What's this change for? It's faster and more memory-efficient to use the slice allocator.

>+soup_cache_entry_free (SoupCache *cache, SoupCacheEntry *entry, gboolean purge)

should be renamed "soup_cache_free_entry()", since the SoupCache comes first. Or swap the arguments around, or store a backpointer to the SoupCache in the SoupCacheEntry so you don't need to pass it.

>+	gchar *uri_str = soup_uri_to_string (uri, FALSE);

plain "char", not "gchar" please. (likewise elsewhere in the patch)

>+	if (!g_hash_table_remove (cache->priv->cache, GUINT_TO_POINTER(entry->key)))

space before (. (likewise elsewhere)

>+		if (!soup_cache_entry_insert_by_key (cache, (const guint) entry->key, entry, TRUE)) {

you don't need that cast

>+	filename = get_filename_from_key (cache, entry->key);
>+	file = g_file_new_for_path (filename);
>+	g_free (filename);

Every place that uses get_filename_from_key() does this same thing. You should change get_filename_from_key() to get_file_for_entry().

>+#define SOUP_CACHE_PHEADERS_FORMAT "(ubuuuuuv)"

Hm... why "v" at the end instead of {ss} like before? (Ah, I see in the code later on. More comments below). As I understand it, if you say "v" there, then it needs to store an extra few bytes of data per entry to say "this variant is an {ss}" each time, so it's better to not do that.

>+		g_file_set_contents (filename, 0, 0, NULL);

first 0 should be NULL

>+	file_enumerator = g_file_enumerate_children_finish (G_FILE (source), result, &error);
>+	if (error) {
>+		if (file_enumerator)
>+			g_object_unref (file_enumerator);

You don't need to check that. file_enumerator will be NULL if error was set.

>+	while ((file_info = g_file_enumerator_next_file (file_enumerator, NULL, NULL)) != NULL) {

It doesn't make a lot of sense to use g_file_enumerate_children_async() but then synchronous g_file_enumerator_next_file(). I think the latter is actually more likely to block than the former.

Since this code only runs in an error case, and probably before the UI is fully up anyway, it's probably OK to just use the sync g_file_enumerate_children() as well.

>+		if (strcmp (filename, SOUP_CACHE_FILE)) {

I prefer to have "!= 0" at the end when testing for a non-match, just to make it a little more obvious when scanning the code quickly.

>+			if (endptr || !g_hash_table_lookup (cache->priv->cache, GUINT_TO_POINTER(key))) {

"*endptr", not "endptr"

>+				g_debug ("removing corrupt file from cache %s", filename);

remove or #ifdef out all g_debug()s before committing please

>+				    &response_time, &hits, &length,
>+                                    &headers_variant)) {

indentation got messed up on the last line somehow. Oh, it's because it uses spaces instead of tabs. Fix that please.

>+		/* If we don't have an array of headers then it means
>+		   that we have corrupt data on the cache. It could be
>+		   that the cache has data in the old format */

But won't it have already failed before getting to this point because of the other changes in PHEADERS_FORMAT? (eg, s->u for key).

(Also, I think that even if it didn't fail because of that, you can't use (ssbuuuuuav) to read something that was written as (ssbuuuuua{ss}), so it would still fail before this point.)

>+		if (!soup_cache_entry_insert_by_key (cache, (const guint) entry->key, entry, FALSE))

again, don't need the cast

>+		cache->priv->lru_start = g_list_reverse (cache->priv->lru_start);
>+		cache->priv->lru_start = g_list_sort (cache->priv->lru_start, lru_compare_func);

Isn't the reverse unnecessary, since the sort call will do the same thing where appropriate? (Or else the sort is unnecessary if the cache file is already stored sorted.)

(In reply to comment #5)
> (From update of attachment 187177 [details] [review])
> And then since it's so easy to deal with multiple patches, you can split your
> commit into separate pieces for each separate change :-) (and do "git bz attach
> 649309 origin/master.." to attach everything you've committed locally that's
> different from upstream).

Sure, we use a similar tool for webkit. I'll give it a try.

> Also, I run all the tests in tests/ under valgrind at least once a release
> cycle, so if you add a cache-test there, then I'll notice leaks myself at some
> point.

Yep, that would be a nice addition indeed.
 
> Anyway...
> 
> >-	entry = g_slice_new0 (SoupCacheEntry);
> >+	entry = g_malloc0 (sizeof(SoupCacheEntry));
> 
> >-	g_slice_free (SoupCacheEntry, entry);
> >+	g_free (entry);
> 
> What's this change for? It's faster and more memory-efficient to use the slice
> allocator.

I think I was just trying some other stuff and left that change there. Will revert it.
 
> >+soup_cache_entry_free (SoupCache *cache, SoupCacheEntry *entry, gboolean purge)
> 
> should be renamed "soup_cache_free_entry()", since the SoupCache comes first.
> Or swap the arguments around, or store a backpointer to the SoupCache in the
> SoupCacheEntry so you don't need to pass it.

Since we could have a lot of entries I think it's better just to swap the arguments in this case.

> >+	gchar *uri_str = soup_uri_to_string (uri, FALSE);
> 
> plain "char", not "gchar" please. (likewise elsewhere in the patch)

Sorry I know that but I always forget :(

> >+	filename = get_filename_from_key (cache, entry->key);
> >+	file = g_file_new_for_path (filename);
> >+	g_free (filename);
> 
> Every place that uses get_filename_from_key() does this same thing. You should
> change get_filename_from_key() to get_file_for_entry().

True
 
> >+#define SOUP_CACHE_PHEADERS_FORMAT "(ubuuuuuv)"
> 
> Hm... why "v" at the end instead of {ss} like before? (Ah, I see in the code
> later on. More comments below). As I understand it, if you say "v" there, then
> it needs to store an extra few bytes of data per entry to say "this variant is
> an {ss}" each time, so it's better to not do that.

I can use the old format with some g_variant_builder_open/close() to "pack" the headers.

> >+	while ((file_info = g_file_enumerator_next_file (file_enumerator, NULL, NULL)) != NULL) {
> 
> It doesn't make a lot of sense to use g_file_enumerate_children_async() but
> then synchronous g_file_enumerator_next_file(). I think the latter is actually
> more likely to block than the former.
> Since this code only runs in an error case, and probably before the UI is fully
> up anyway, it's probably OK to just use the sync g_file_enumerate_children() as
> well.

Not sure about which one could more likely block, thing is that I was trying to potentially avoid creating a lot of async operations (one per file). But yeah, I agree that being an unlikely error situation it does not pay off trying to optimize it. 
 
> >+		if (strcmp (filename, SOUP_CACHE_FILE)) {
> 
> I prefer to have "!= 0" at the end when testing for a non-match, just to make
> it a little more obvious when scanning the code quickly.

I'll change that

> >+		/* If we don't have an array of headers then it means
> >+		   that we have corrupt data on the cache. It could be
> >+		   that the cache has data in the old format */
> 
> But won't it have already failed before getting to this point because of the
> other changes in PHEADERS_FORMAT? (eg, s->u for key).

I thought the same but it turns out that it does not, maybe GVariant expects something more for arrays.
 
> (Also, I think that even if it didn't fail because of that, you can't use
> (ssbuuuuuav) to read something that was written as (ssbuuuuua{ss}), so it would
> still fail before this point.)

That's what common sense says, but the fact is that GVariant does not perform many checks and allows you to do things like that. Check out this basic example: http://pastebin.com/T6JhpgPx, it runs smoothly. Thing is that there is no way to check the format used to store some data AFAIK, I tried using g_variant_is_of_type but it didn't complain either.

> >+		cache->priv->lru_start = g_list_reverse (cache->priv->lru_start);
> >+		cache->priv->lru_start = g_list_sort (cache->priv->lru_start, lru_compare_func);
> 
> Isn't the reverse unnecessary, since the sort call will do the same thing where
> appropriate? (Or else the sort is unnecessary if the cache file is already
> stored sorted.)

Yeah I guess we can get rid off the sorting.

Created attachment 187278 [details] [review]
Patch v3

New version with the changes suggested by Dan.

Created attachment 187569 [details] [review]
Fix for the leaks

The patch was starting to grow trying to fix several different issues at the same time. Seems like a good idea to split it a bit. This first one just fixes the memory leaks without any change in the format.

Created attachment 187570 [details] [review]
Delete invalid cache files

Deletes cache files when the cache cannot successfully read the cache contents.

Once we get these in, I'll file a new bug for some modifications in the format used to store the cache data on disk.

Comment on attachment 187569 [details] [review]
Fix for the leaks

>+#include <string.h>

I don't think that's needed for this patch?

> 	if (!g_list_length (cache->priv->lru_start))
>-		return;
>+ 		return;

looks like an extra space got inserted before the tabs on that line

>+	uint freshness_lifetime, hits;

"uint" isn't a standard type name, so that would probably fail on solaris or something. So use "guint" for that. (The rule is "use g-types where it results in less typing than the (portable) non-g-type, but not where it results in more". So "int", "char", but "guint" instead of "unsigned int", etc.)

>+	g_debug ("Loading %u items from cache", items);

still get rid of that please

>+		/* Check that we have headers */
>+		soup_message_headers_iter_init (&soup_headers_iter, headers);
>+		if (!soup_message_headers_iter_next (&soup_headers_iter, &header_key, &header_value)) {
>+			soup_message_headers_free (headers);
>+			continue;
>+		}

This is weird... it seems like it would be easier to just set a gboolean to TRUE if you reach the soup_message_headers_append() call. Or else add soup_message_headers_get_length().


The leak fixing all looks right.

Comment on attachment 187570 [details] [review]
Delete invalid cache files

>+			if (strcmp (filename, SOUP_CACHE_FILE) != 0) {

ah, yeah, it's this patch that should have the "#include <string.h>"

>+		/* If we don't have an array of headers then it means
>+                 * that we have corrupt data on the cache.
>+		 */

spaces vs tabs

>+		if (g_variant_iter_n_children (headers_iter) == 0)
>+			continue;

Isn't this check redundant with the check I didn't like in the last patch? So you could just rid of the previous check. Also, if filename is valid, should you delete the file it points to here? (And then the g_list_length() check at the end should be unnecessary too.)

(In reply to comment #12)
> (From update of attachment 187570 [details] [review])

> >+		if (g_variant_iter_n_children (headers_iter) == 0)
> >+			continue;
> 
> Isn't this check redundant with the check I didn't like in the last patch? So
> you could just rid of the previous check. Also, if filename is valid, should
> you delete the file it points to here? (And then the g_list_length() check at
> the end should be unnecessary too.)

I think the point here is how do we want to deal with changes in the cache format that will eventually happen in the future. Taking into account that we cannot trust GVariant, what about an easy fix like saving the format used in by the cache in another file? Then we could check if the format used by the cache is the same than the one built in, and if that is not the case, then we'd proceed to delete the cache contents.

How about just changing the name of "soup.cache" to "soup.cache2"? And including a version field at the start in the new format.

(In reply to comment #14)
> How about just changing the name of "soup.cache" to "soup.cache2"? And
> including a version field at the start in the new format.

Sounds good. After thinking about it twice I think we do not need to save the format because if that changes the code will be different too.

Committed 15c9ad908b4758b4e976bd27d14ce82847133e5e. We will handle the cache format issues in a different bug.