GNOME Bugzilla – Bug 733170
downloader doesnt free memory
Last modified: 2016-03-31 13:22:07 UTC
It seems the downloader doesn't frees the memory. I noticed this while testing around for bug #729026 and noticed that Boxes, after downloading an 1.6GiB ISO, takes 3.4GiB of RAM in the memory. (With no active VM and the download definitely finished!) We should adress this issue before merging the patches from bug #729026.
Oh, this also reminds me that current download code was made for small files, not GBs of data. We also need to modify that as part of bug#729026.
See soup_message_body_set_accumulate()
IIUC set_accumulate (false) will make the message object delete every buffer after calling the got_chunk signal. (Tried this out, it seems so.) If I call it after we copy everything out of the buffers it seems to have no effect. So if we want to use this we'd need to copy all bytes from the buffer in got_chunk into an own buffer and basically building up our own buffer this way. That seems neither elegant nor a good solution to me but I didn't found something else so far.
(In reply to comment #3) > IIUC set_accumulate (false) will make the message object delete every buffer > after calling the got_chunk signal. (Tried this out, it seems so.) > > If I call it after we copy everything out of the buffers it seems to have no > effect. > > So if we want to use this we'd need to copy all bytes from the buffer in > got_chunk into an own buffer and basically building up our own buffer this way. No :) The point is to write the buffer out as soon as you receive it, not to recreate the issue in the app. :)
Created attachment 280742 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Review of attachment 280742 [details] [review]: This is just a draft and it works for me, tested it with my iso downloading patches. The issues are in the inline comments. ::: src/downloader.vala @@ +132,2 @@ current_num_bytes += chunk.length; + cached_file_stream.write (chunk.data); Throws an exception. IIUC I have to handle it here. I'd do session.cancel_message (msg, Soup.Status.NONE); However it seems that there is no suitable Soup.Status available and its also not really meant for that kind of error. (See http://valadoc.org/#!api=libsoup-2.4/Soup.Status) I don't see how else to stop Soup from downloading since cancel_message is obviously meant for that. (http://valadoc.org/#!api=libsoup-2.4/Soup.Session.cancel_message) @@ +141,3 @@ yield; + if (msg.status_code != Soup.Status.OK) { + download.cached_file.delete (); This will not be invoced when the user terminates Boxes. Will a try-finally clause do? I tried it out and got a weird error when having only (!) download.cached_file.delete (); in a finally block: error: jump out of finally block not permitted I don't see how I jump out of there, maybe I just need someone to open my eyes, if you don't see my fault either by just reading the text then its best I do my research myself instead of wasting your time.
Review of attachment 280742 [details] [review]: ::: src/downloader.vala @@ +132,2 @@ current_num_bytes += chunk.length; + cached_file_stream.write (chunk.data); well, apart from cancelling the message, you can have a local error variable in the outer block and set it from here. After 'yield' below, you can check if that error is non-null and throw it, if so. @@ +141,3 @@ yield; + if (msg.status_code != Soup.Status.OK) { + download.cached_file.delete (); I don't understand how try-finally will address the issue of Boxes exiting. I think the operation should be cancelled on Boxes exit and Boxes should wait for cancellation to actually happen before it exits (i-e this async call is done).
Review of attachment 280742 [details] [review]: Thanks for the suggestions! I'll update this soon. ::: src/downloader.vala @@ +141,3 @@ yield; + if (msg.status_code != Soup.Status.OK) { + download.cached_file.delete (); aren't finally blocks there to be executed independent from whats happening? I think that should include exiting the program.
Review of attachment 280742 [details] [review]: ::: src/downloader.vala @@ +141,3 @@ yield; + if (msg.status_code != Soup.Status.OK) { + download.cached_file.delete (); yes but you are supposed to get a cancelled error here and you'll want to handle that anyway so no need to bother with finally AFAICT.
Created attachment 280824 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Review of attachment 280824 [details] [review]: ::: src/downloader.vala @@ +144,3 @@ download_from_http.callback (); }); + There is a trailing whitespace.
Review of attachment 280824 [details] [review]: ::: src/downloader.vala @@ +144,3 @@ download_from_http.callback (); }); + Bugzilla shows that for every empty line. It isnt really there ;)
Review of attachment 280824 [details] [review]: Good otherwise. ::: src/downloader.vala @@ +123,3 @@ }); + var cached_file_stream = download.cached_file.append_to (FileCreateFlags.NONE); why append? If the file already exists, we either want to override it or skip the downloading. @@ +144,3 @@ download_from_http.callback (); }); + I think Lasse just added a new line. @@ +152,1 @@ throw new Boxes.Error.INVALID (msg.reason_phrase); since previous if is not about exiting earlier, this should be in an 'else'.
Review of attachment 280824 [details] [review]: ::: src/downloader.vala @@ +123,3 @@ }); + var cached_file_stream = download.cached_file.append_to (FileCreateFlags.NONE); because I can't read documentation how it is meant to be read :) in practice this doesnt make any difference because if the file exists this statement will never be executed but for maintainability you're totally right, this should be create instead of append_to. @@ +152,1 @@ throw new Boxes.Error.INVALID (msg.reason_phrase); Throw, like return, exits the function. I'm sure if it were a return statement and I'd put an else there you'd say thats unnecessary indentation. How does this differ?
Review of attachment 280824 [details] [review]: ::: src/downloader.vala @@ +152,1 @@ throw new Boxes.Error.INVALID (msg.reason_phrase); both blocks throw/exit so thats no diff between two blocks. No, I'm sure I wouldn't say that in this case because: 1. Its not a big block being indented and there isn't multiple levels of indentation either. 2. This is not about returning earlier since we are throwing error in both cases here and if is only there to decide which error to throw. Here is an example of returning earlier: if (condition) throw error; x = y; do_something ();
Created attachment 280871 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Review of attachment 280871 [details] [review]: ::: src/downloader.vala @@ +148,3 @@ + if (msg.status_code != Soup.Status.OK) { + download.cached_file.delete (); + if (err != null) you wanted '==' here.
Created attachment 280922 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +123,3 @@ }); + var cached_file_stream = download.cached_file.create (FileCreateFlags.NONE); just realized that you are using the sync versions here. You need to use async version. @@ +133,3 @@ current_num_bytes += chunk.length; + try { + cached_file_stream.write (chunk.data); Same here. I know that it'll make this call complicated but ideally all I/O must be async. @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); I only just remembered that you can use libsoup's HTTP status codes as error codes. See documentation for SOUP_HTTP_ERROR.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +133,3 @@ current_num_bytes += chunk.length; + try { + cached_file_stream.write (chunk.data); so it does make a difference if I invoce write () or yield write_async ()? @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); SOUP_HTTP_ERROR doesn't help _me_ much so far. I'll spend some more time in the documentation for this and maybe ask more about it later.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +133,3 @@ current_num_bytes += chunk.length; + try { + cached_file_stream.write (chunk.data); of course, one is a sync call and the other is async and will make us enter mainloop while its writing data. @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); How come? its not binded? should be super easy if its binded.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +133,3 @@ current_num_bytes += chunk.length; + try { + cached_file_stream.write (chunk.data); The day may come when I finally and unmisunderstandably get how these vala async things are working... but I'm learning. @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); SOUP_HTTP_ERROR seems to be a macro to soup_http_error_quark(). The former has no vala binding while the latter has. This is some function returning this weird "quark" thingy I never heard anything about yet and that somehow assigns a number to some strings ;)
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); There seems to be some confusion (based on our private chat) about this. I'm not saying that you shouldn't use session.cancel_message (msg, Soup.Status.CANCELLED) some lines above. Since err is NULL here, it means we didn't get error from writing the chunk to file nor we canceled it. So it must have been an error from soup and therefore we should ideally create an error from soup's status code rather than creating a generic one.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); That sounds pretty much like an own commit to me and therefore wouldnt belong in this bug.
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); its very much about this patch. Read what I'm talking about and see how its about the code you introduced here. :)
Review of attachment 280922 [details] [review]: ::: src/downloader.vala @@ +149,3 @@ + download.cached_file.delete (); + if (err == null) + err = new Boxes.Error.INVALID (msg.reason_phrase); I'm sorry but I think I'm still not getting what you want to express. What I did understand: - line 138 (cancel_message) is ok :) - you suggest I shouldnt do err = new Boxes.Error.INVALID (msg.reason_phrase); because it is too generic. - Thats what happens in the original code also, line 138 in the original, thats why I said this belongs not to this patch IMO - I had to map every value of Soup.Status to an appropriate exception to do that. That doesnt make much sense to me. It would probably be way easier to make use of Soup.Status.get_phrase and/or use the msg.reason_phrase. - I don't know if this has still anything to do with SOUP_HTTP_ERROR but I assume this is irrelevant now.
Created attachment 281843 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Created attachment 281856 [details] [review] downloader: Write chunks to file instead of memory I modified the comments for you and was going to commit but then I saw this: downloader.vala:138.13-138.49: warning: unhandled error `GLib.IOError' cached_file_stream.write (chunk.data); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Please ensure that your patches don't introduce build or runtime warnings/errors before submitting them.
Review of attachment 281856 [details] [review]: ::: src/downloader.vala @@ +136,3 @@ + // calls and we'll end up writing bytes out in wrong order. Besides + // we are writing small chunks so it whouldn't really block the UI. + cached_file_stream.write (chunk.data); yeah, sorry. I somehow removed the try catch things when trying the async things. New patch comes shortly.
Created attachment 281858 [details] [review] downloader: Write chunks to file instead of memory This makes the downloader suitable for downloading large files and additionally prevents that all downloaded data is hold in the memory until the program terminates.
Attachment 281858 [details] pushed as 3eabdb1 - downloader: Write chunks to file instead of memory