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

Submitter:  kempston <kempston>
Submitted:  Wed 20 Apr 2005 11:50:15 PM UTC
   
 
Category:  Core Severity:  5 - Blocker
Item Group:  Program malfunction Status:  Ready For Test
Assigned to:  None Open/Closed:  Closed
Release:  2.5.30.14 Release: 
Operating System:  Linux Binaries Origin:  CVS / Self compiled
CPU type:  Intel x86

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>
Group administrator
Sun 21 Aug 2005 12:23:54 PM UTC, comment #15: 

Copied your patch to patch #4334

spiralvoice <spiralvoice>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 15 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2005-09-20 spiralvoice Reassign ItemFrom group mldonkey, a multi-networks file-sharing client, bugs tracker To group mldonkey, a multi-networks file-sharing client, patch tracker
2005-09-09 kempston Attached File- Added 2.6.4-broken-cancel.patch, #2938
2005-09-09 kempston Attached File- Added 2.6.4-broken-cancel.patch, #2935
2005-09-07 kempston Attached File- Added 2.6.4-broken-cancel.patch, #2922
2005-09-07 kempston Attached File- Added 2.6.4-broken-cancel.patch, #2921
2005-08-21 spiralvoice StatusFixed Ready For Test
    Open/ClosedClosed Open
2005-08-21 kempston Attached File- Added cancel-2.6.2.patch, #2846
2005-08-17 spiralvoice StatusDuplicate Fixed
    Open/ClosedOpen Closed
2005-06-10 spiralvoice Attached File- Added cancel.patch, #2588
2005-05-29 amorphous StatusNone Duplicate
2005-04-28 spiralvoice Severity3 - Normal 5 - Blocker
    Item GroupSegmentation Fault Program malfunction
    Release2-5-30 2.5.30.14

Back to the top

Powered by Savane 3.13-d3ae.
Corresponding source code