Client terminates when removing 300 or more items from Finished Downloads

Bug #261618 reported by LoRenZo
12
Affects Status Importance Assigned to Milestone
DC++
Fix Released
High
Unassigned

Bug Description

As the title says, the client (v0.707) closes itself without giving any error messages when i remove too many items from the Finished Download tab. No exeptioninfo.txt is created after that. Happened twice, the last time i saw that there were 302 items in the list. It could also affect the Finished Uploads tab.

eMTee (realprogger)
Changed in dcplusplus:
importance: Undecided → High
status: New → Confirmed
Revision history for this message
eMTee (realprogger) wrote :

Tested with bzr1423, delete ~300 out of 315 (filelist) items from the Grouped by files list. Tried twice, once it crashed, not for 2nd time :P

Revision history for this message
LoRenZo (lorenzo-mailbox-deactivatedaccount) wrote :

I think this could be in context with active downloads, because the last time i tried to remove a few (about 15) files from the list (altogether 301 files), dc has crashed immediately. (I used to remove files in small groups - not more then 50 items -, which didn't lead to crashing the application.) As i remember, the total download speed was about 500KB/s, and there were 5 or 6 downloads running. Before that event, i could remove all 815 files from the list without crashing the program, but there was no active traffic (neither download nor upload), and all hubs were closed. I hope these informations will be useful. By the way, i'm still using v0.707, because v0.708 is very unstable for me (i hope that "small" bug will be fixed in it soon, hopefully with this one too).

poy (poy)
Changed in dcplusplus:
status: Confirmed → Fix Committed
Revision history for this message
Steven Sheehy (steven-sheehy) wrote :

I don't agree with the changes made to fix this defect. The changes modify the core API used by different clients (like LinuxDC++) in a way that is not consistent with the rest of the core API. It removes the MVC type notification that is used by every implementer of the core listeners so that the view no longer receives notification that the model has changed. Also, it is not proper to so dramatically change the core API in a minor release and without notifying impacted clients. This needs further discussion.

Changed in dcplusplus:
status: Fix Committed → In Progress
Revision history for this message
Jacek Sieka (arnetheduck) wrote :

indeed, the notification must stay...it's some sort of timing issue that needs more love...

Revision history for this message
poy (poy) wrote :

the cause of the crash is that when removing several items at the same time, async calls are raised for each removed item; then when the first of these calls is processed, it doesn't know that other items have also been removed.

specifically, when removing many items, updateStatus is called from onRemovedFile/onRemovedUser and loops through all current items displayed in the list. it crashes as soon as it tries to get info for an item that has already been removed.

another (easier) way of reproducing this is to right-click on the list, wait for transfers to complete, then select "Remove all": newly finished items that were going to be added by the async onAddedFile/onAddedUser are deleted, which leads to a crash when one of these calls tries to run.

my fix was to make all these calls synchronous (the "remove a finished item" event can only be raised by that view anyway), which led to unused listeners in the core that i removed too.
as an alternate solution (unless someone can think of a better way), my fix could be applied to win32/ only; mods would still be able to use these events as before.

Revision history for this message
poy (poy) wrote :

gave this another thought.

attached patch makes the call to "remove all" asynchronous so that when the menu is closed, the list is given a chance to add new items before removing them.
solves the crash on "remove all".

i don't know if the crash when removing many items is still present because it didn't occur when i tried; if it does, then i had these possible solutions in mind:
- re-apply the win32 part of my previous fix.
- make the call to "remove" async too.
- store sizes / other relevant info that the status bar needs in each item in the GUI list so that the status bar can still update itself if the pointer to FinishedItem is invalidated.

Revision history for this message
poy (poy) wrote :

fixed differently by preventing finished items pointers from being deleted if they're still in use (eg, by the GUI list that hasn't had time to update itself).

there were actually many more issues related than just the ones mentioned here, and none of the previous patches solved all of them... the 1st one even didn't make any sense, thanks for noticing Steven.

one such "side issue" is a user that would click an item once, select "Remove", then re-click the item before it has been removed and select another command... hopefully fixed now, i've only tested "Remove all" though.

Changed in dcplusplus:
status: In Progress → Fix Committed
eMTee (realprogger)
Changed in dcplusplus:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Patches

Remote bug watches

Bug watches keep track of this bug in other bug trackers.