bugmldonkey, a multi-networks file-sharing client - Bugs: bug #12761, Cancel is broken

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #12761: Cancel is broken

Submitted by:  kempston <kempston>
Submitted on:  Wed 20 Apr 2005 11:50:15 PM UTC  
 
Category: CoreSeverity: 5 - Blocker
Item Group: Program malfunctionStatus: Ready For Test
Assigned to: NoneOpen/Closed: Closed
Release: Release: 2.5.30.14
Operating System: LinuxBinaries Origin: CVS / Self compiled
CPU type: Intel x86

(Jump to the original submission Jump to the original submission)

Fri 09 Sep 2005 08:07:05 AM UTC, comment #28:

Here is even more improved version.
As additional to mentioned 1,2,3 it also has
4. preventing exception on canceling bittorent with torrent file being somehow removed (i had plenty of them)

kempston <kempston>
Fri 09 Sep 2005 02:54:11 AM UTC, comment #27:

Here is improoved version of the patch.
1. Now removing swarmer in file_completed is also purged.
2. When file has no swarmer (completed) canceling does not produce exception.
3. Still no multinetwork support of course and thus removed senless parts of code.

kempston <kempston>
Thu 08 Sep 2005 05:29:08 PM UTC, comment #26:

Hmm... code should be rewritten then :)
I guess to make it working we need to add swarmer filed into CommonFile.file
I'm not using multinet so i'm happy even with this patch, maybe someone else want to finish it

kempston <kempston>
Thu 08 Sep 2005 04:20:06 PM UTC, comment #25:

+ if !found = 0 then Int64Swarmer.remove_swarmer file_swarmer

This is checking to see if it was found in current_files.

current_files is module specific.

If a swarmer is shared by one file in bittorrent, and one file in donkey, aren't you removing the swarmer from swarmers_by_name for the donkey file if you cancel the bittorrent file?

Anonymous
Thu 08 Sep 2005 03:26:31 PM UTC, comment #24:

Wrong, wrong, wrong.

>It isn't checking to see that the swarmer isn't in use by some other file.

Actually it does, that's the point. What do you think this is for?
+ if !found = 0 then Int64Swarmer.remove_swarmer file_swarmer

>Once you set file.file_swarmer <- None for every file that is using the particular swarmer, the garbage collector will automatically remove the entry from swarmers_by_name since no one else references it anymore.


If you have two files downloading, using the same swarmer, another file is referencing to same swarmer, even after cancelling one of them.

kempston <kempston>
Thu 08 Sep 2005 03:18:16 PM UTC, comment #23:

What if you have two files downloading, using the same swarmer, and you cancel one of them? Won't your patch forcefully remove the swarmer and break the download of the other file? It isn't checking to see that the swarmer isn't in use by some other file.

The swarmers_by_name in commonSwarming2 is a Weak Hashtable. (a design decision of some sort)

Once you set file.file_swarmer <- None for every file that is using the particular swarmer, the garbage collector will automatically remove the entry from swarmers_by_name since no one else references it anymore.

Since you have to wait ~2 minutes after cancelling, this means the garbage collector (GC) didn't yet run.

If you add a call to "Gc.compact ();" in file_cancel (commonInteractive.ml after "impl.impl_file_ops.op_file_cancel impl.impl_file_val;" ) it should force the garbage collection immediately after a cancel. This is very ugly and slows down each call to file_cancel while garbage is collected.

You can test this theory by 1. starting a download, 2. cancelling it, 3. running "mem_stats -1", and then 4. trying to redownload the same file.
(without using your patch)

( mem_stats called with a negative integer parameter calls the garbage collector. )

Anonymous
Wed 07 Sep 2005 10:01:36 PM UTC, comment #22:

>Why is found in commonTypes? It doesn't belong.
>Why aren't you using List.exists instead of found?

Couse my Ocaml knowledge sucks

>There is duplication of code

Yes couse this code won't work from CommonSwarming.

>and it doesn't apply to all networks

Yes, couse i only care about bt/ed2k/filetp

>so this patch is probably wrong.

Well you might want to improove it in some way but it does it's job

kempston <kempston>
Wed 07 Sep 2005 02:15:59 PM UTC, comment #21:

@2.6.4-broken-cancel.patch

Why is found in commonTypes? It doesn't belong.

Why aren't you using List.exists instead of found?

There is duplication of code, and it doesn't apply to all networks, so this patch is probably wrong.

Anonymous
Wed 07 Sep 2005 08:54:32 AM UTC, comment #20:

This one works fine

kempston <kempston>
Wed 07 Sep 2005 07:56:15 AM UTC, comment #19:

Oh, well it's broken

kempston <kempston>
Wed 07 Sep 2005 04:08:27 AM UTC, comment #18:

This patch finnaly solves the problem
I really wish i didn't have to write it

kempston <kempston>
Tue 06 Sep 2005 10:27:50 PM UTC, comment #17:

Seems like with large number of downloads patch not doing it's job at all

kempston <kempston>
Sun 21 Aug 2005 12:28:40 PM UTC, comment #16:

Applied your patch to CVS

spiralvoice <spiralvoice>
Project Administrator
Sun 21 Aug 2005 12:23:54 PM UTC, comment #15:

Copied your patch to patch #4334

spiralvoice <spiralvoice>
Project Administrator
Sun 21 Aug 2005 10:58:35 AM UTC, comment #14:

Well here is same patch for bt and filetp.

kempston <kempston>
Wed 17 Aug 2005 12:45:51 AM UTC, comment #13:

Not sure if it can be closed, i just checked last 2.6.2 and "fast readding" problem is still there, and from what i remember it's just a symptom of several important problems

kempston <kempston>
Sat 11 Jun 2005 07:53:10 PM UTC, comment #12:

Opened patch #4091 with the file cancel.patch from here.

spiralvoice <spiralvoice>
Project Administrator
Fri 10 Jun 2005 03:19:30 PM UTC, comment #11:

Did some tests, I don“t think "file.file_swarmer <- None;" will
help, the problem seems to be in commonFile. All tests where
done with a patched core.

Start the core in an empty dir, add an ed2k-link

mem_stats 1
Module CommonFile
files: 1
Module CommonSwarming2
Swarmers: 1

cancel the download

mem_stats 1
Module CommonFile
files: 1
Module CommonSwarming2
Swarmers: 1

10 seconds later

mem_stats 1
Module CommonFile
files: 1
Module CommonSwarming2
Swarmers: 0

add three other files and cancel them immediatly afterwards

mem_stats 1
Module CommonFile
files: 5
Module CommonSwarming2
Swarmers: 0

The swarmers took about two minutes to be cleaned, one swarmer
was removed immediatly after canceling, the two others took
the two minutes you mentioned, so this has nothing to do with
save_options_delay, weird...

But nonetheless in commonFile the files are never removed...
Although after save in files.ini there are no entries left, so
"file.file_swarmer <- None;" helped a bit indeed. I attached the
patch for it here.

spiralvoice <spiralvoice>
Project Administrator
Fri 10 Jun 2005 03:03:05 PM UTC, comment #10:

Well, i'm not sure about that - thing is, if you do manual save (even ten times), it won't clear the list.
Anyway, i think producing patch should come after fixing 2nd issue. Really hope to hear some wise adwise from someone like Amorphous :)

kempston <kempston>
Fri 10 Jun 2005 02:56:10 PM UTC, comment #9:

You may be right about "file.file_swarmer <- None;"
from gnutellaClients.ml:

let download_finished file =
if List.memq file !current_files then begin
file_completed (as_file file);
file.file_swarmer <- None;

So the code is used there, too. This should be patched to have a
first solution. Regarding the postponed cleaning of the swarmer
list, I think it is related to save_options_delay, so it may be
longer than 2 minutes.

spiralvoice <spiralvoice>
Project Administrator
Fri 10 Jun 2005 10:16:49 AM UTC, comment #8:

Hmm, i remember myself checking commit code before, didn't find there anyhting userfull, but thnx for the great tip - i've checked some traces and found out, that in donkeyOneFile.ml in declare_completed_file function there is a code "file.file_swarmer <- None;" that can be used as i kind of solution.

Putiing this code into donkeyInteractive.ml into begining of file_ops.op_file_cancel function makes things working with one huge BUT.

if you start downloading, then cancel it immideatly file swarmer get's released, but list of swarmers still remembers it, if you start another donwnload immediatly it will tell you, you're the moron, as usually, but if you will restart core or wait 2 minutes list will get updated and everything will start working fine.

I've trying to fix this issue and located follwing code in commonSwarming2.ml

let swarmers =
define_option CommonComplexOptions.swarmers_section
["swarmers"] "All the swarmers used" (list_option SwarmerOption.t) []

and

let _ =
set_after_save_hook files_ini (fun _ -> swarmers =:= []);
set_before_save_hook files_ini (fun _ ->
let list = ref [] in
HS.iter (fun s ->
if s.s_networks <> [] then
list := s :: !list) swarmers_by_name;
swarmers =:= !list
);
set_after_load_hook files_ini (fun _ ->
List.iter (fun s ->
check_swarmer s;
) !!swarmers;

swarmers =:= [])

2nd part seems to be resposible for updating swarmers list, but list itself is unaccessable from any other part of code :(

Unfortunatly my ocaml knowledge is not strong enought to get the trick done, so i'd really apretiate help for one of big brothers.

kempston <kempston>
Wed 08 Jun 2005 12:11:38 AM UTC, comment #7:

Note for devs: The swarmers section is removed when a file is committed. Maybe the necessary code section to be added to the
cancel command can be found in the commit code.

spiralvoice <spiralvoice>
Project Administrator
Fri 03 Jun 2005 04:50:04 PM UTC, comment #6:

Ok, i've discovered following: after starting download and then canceling it i can see this in files.ini

swarmers = [
{ file_size = 3926800
file_name = "./temp/urn_ed2k_AE9DEC30E5BC22F82061CEBB6A7D786B"
file_bitmap = 1
file_chunk_sizes = [
9728000;]
};]

i belive this should be removed on cancel, and in sources in file_ops.op_file_cancel i can't see anything like swarmer.remove. More than that, i can't find any similiar function in commonSwarming2.ml anyone can comment that?

kempston <kempston>
Sun 29 May 2005 06:40:03 PM UTC, comment #5:

duplicate of savannah bug #12372

Amorphous <amorphous>
Fri 29 Apr 2005 10:05:15 AM UTC, comment #4:

or does it actually start a new download?
see savannah bug #12372 for what i reffered to in the last comment.

Amorphous <amorphous>
Fri 29 Apr 2005 09:18:02 AM UTC, comment #3:

afaik it's not a dublicate. this one is about that there remain traces of removed downloads and canceled downloads in mldonkey (until it is restarted). the other one is about exceptions when iterating networks, so the common code thinks that network (that didn't return false because of an exception) accepted the download and only starts the download as a non-primary in any following networks.

Amorphous <amorphous>
Thu 28 Apr 2005 12:35:32 PM UTC, comment #2:

Continuing in bug #12841 which is a somehow a duplicate.

spiralvoice <spiralvoice>
Project Administrator
Thu 28 Apr 2005 12:10:09 PM UTC, comment #1:

I could reproduce that bug:

start mlnet in a clean dir, lots of networks enabled:
Networks:

8 Donkey Enabled
7 BitTorrent Enabled
4 G2 Enabled
5 Fasttrack Disabled
2 Open Napster Enabled
3 Gnutella Enabled
6 FileTP Enabled
9 Soulseek Enabled

Add an ed2k download with dllink command, everything is normal.
Now cancel it via HTML interface and save ini files.
The file data in the swarmers[] section in files.ini is not
removed, the file in ./temp is removed.
Now re-enter the same dllink command:
"Exception File "src/daemon/common/commonSwarming2.ml", line 0, characters 609-14: Assertion failed for network Donkey
Exception Not_found for network FileTP
Unable to match URL"
the file in ./temp is created and files.ini is updated, but
not in the files[] section, instead of that a line has been
added to file_chunk_sizes, normally (after the first dllink)
it look like this:

file_chunk_sizes = [
9728000;]

after the second dllink with its error message it looks like this:

file_chunk_sizes = [
9728000;
9728000;]

spiralvoice <spiralvoice>
Project Administrator
Wed 20 Apr 2005 11:50:15 PM UTC, original submission:

If you add ed2k download, then cancel it and then add again, you will get
Exception Failure("File already downloaded (use 'force_download' if necessary)") for network Donkey
Exception Not_found for network FileTP
Unable to match URL

also on restart i've noticed that such downloads are recovered by temp_recover(so i guess problem is it's not eraiseing temp file)

Works fine in 2-5-28-1. After moving to any of 2-5-30 bug is showing it's face.

kempston <kempston>

 

Attached Files
file #3306:  2.6.4-broken-cancel.patch added by kempston (6KiB - application/octet-stream)
file #3318:  2.6.4-broken-cancel.patch added by kempston (6KiB - application/octet-stream)
file #3455:  2.6.4-broken-cancel.patch added by kempston (7KiB - application/octet-stream)
file #3454:  2.6.4-broken-cancel.patch added by kempston (5KiB - application/octet-stream)
file #3385:  cancel-2.6.2.patch added by kempston (982B - application/octet-stream - cancel patch for bt and filetp)
file #3072:  cancel.patch added by spiralvoice (454B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Do you think this task is very important?
If so, you can click here to add your encouragement to it.
This task has 0 encouragements so far.

Only logged-in users can vote.

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 15 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Tue 20 Sep 2005 09:03:57 PM UTCspiralvoiceReassign ItemFrom group mldonkey, a multi-networks file-sharing client, bugs tracker=>To group mldonkey, a multi-networks file-sharing client, patch tracker
Fri 09 Sep 2005 08:07:05 AM UTCkempstonAttached File-=>Added 2.6.4-broken-cancel.patch, #2938
Fri 09 Sep 2005 02:54:13 AM UTCkempstonAttached File-=>Added 2.6.4-broken-cancel.patch, #2935
Wed 07 Sep 2005 08:54:32 AM UTCkempstonAttached File-=>Added 2.6.4-broken-cancel.patch, #2922
Wed 07 Sep 2005 04:08:27 AM UTCkempstonAttached File-=>Added 2.6.4-broken-cancel.patch, #2921
Sun 21 Aug 2005 12:23:54 PM UTCspiralvoiceStatusFixed=>Ready For Test
  Open/ClosedClosed=>Open
Sun 21 Aug 2005 10:58:38 AM UTCkempstonAttached File-=>Added cancel-2.6.2.patch, #2846
Wed 17 Aug 2005 12:19:51 AM UTCspiralvoiceStatusDuplicate=>Fixed
  Open/ClosedOpen=>Closed
Fri 10 Jun 2005 03:19:30 PM UTCspiralvoiceAttached File-=>Added cancel.patch, #2588
Sun 29 May 2005 06:40:03 PM UTCamorphousStatusNone=>Duplicate
Thu 28 Apr 2005 12:10:28 PM UTCspiralvoiceSeverity3 - Normal=>5 - Blocker
  Item GroupSegmentation Fault=>Program malfunction
  Release2-5-30=>2.5.30.14

Back to the top


Powered by Savane 3.1-cleanup1