Crash when adding search result from user who has left the hub

[Razzloss]

Linuxdcpp crashes when trying to download a search result from a user who has left the hub. The crash doesn't happen if the result is added to the queue immediately after the user leaves, but minute or two should be enough, after that attempt to add result to the queue leads to crash (or assert failure with debug builds).

BT From debug build:
#0 0xb7f73424 in __kernel_vsyscall ()
#1 0xb75c1660 in raise () from /lib/libc.so.6
#2 0xb75c2e98 in abort () from /lib/libc.so.6
#3 0xb75ba6ce in __assert_fail () from /lib/libc.so.6
#4 0x0809fd8b in boost::intrusive_ptr<dcpp::User>::operator* (this=0xb48bf284) at /usr/include/boost/intrusive_ptr.hpp:120
#5 0x0809fda3 in dcpp::User::Hash::operator() (this=0x8f918ce, x=@0xb48bf284) at dcpp/User.h:64
#6 0x082138fb in std::tr1::__detail::_Hash_code_base<boost::intrusive_ptr<dcpp::User>, std::pair<boost::intrusive_ptr<dcpp::User> const, dcpp::QueueItem*>, std::_Select1st<std::pair<boost::intrusive_ptr<dcpp::User> const, dcpp::QueueItem*> >, std::equal_to<boost::intrusive_ptr<dcpp::User> >, dcpp::User::Hash, std::tr1::__detail::_Mod_range_hashing, std::tr1::__detail::_Default_ranged_hash, false>::_M_hash_code (this=0x8f918cc,
  __k=@0xb48bf284) at /usr/lib/gcc/i686-pc-linux-gnu/4.3.2/include/g++-v4/tr1_impl/hashtable_policy.h:697
#7 0x08213916 in std::tr1::_Hashtable<boost::intrusive_ptr<dcpp::User>, std::pair<boost::intrusive_ptr<dcpp::User> const, dcpp::QueueItem*>, std::allocator<std::pair<boost::intrusive_ptr<dcpp::User> const, dcpp::QueueItem*> >, std::_Select1st<std::pair<boost::intrusive_ptr<dcpp::User> const, dcpp::QueueItem*> >, std::equal_to<boost::intrusive_ptr<dcpp::User> >, dcpp::User::Hash, std::tr1::__detail::_Mod_range_hashing, std::tr1::__detail::_Default_ranged_hash, std::tr1::__detail::_Prime_rehash_policy, false, false, true>::find (this=0x8f918cc, __k=@0xb48bf284)
at /usr/lib/gcc/i686-pc-linux-gnu/4.3.2/include/g++-v4/tr1_impl/hashtable:762
#8 0x08202771 in dcpp::QueueManager::UserQueue::getRunning (this=0x8f9180c, aUser=@0xb48bf284) at dcpp/QueueManager.cpp:273
#9 0x08207dd7 in dcpp::QueueManager::addSource (this=0x8f91708, qi=0x978b000, aUser=@0xb48bf284, addBad=991) at dcpp/QueueManager.cpp:685
#10 0x0820de72 in dcpp::QueueManager::add (this=0x8f91708, aTarget=@0xb48bf2b8, aSize=4696548889, root=@0xb48bf2a0, aUser=@0xb48bf284,
  hubHint=@0xb48bf31c, aFlags=0, addBad=true) at dcpp/QueueManager.cpp:626
#11 0x080ed3bf in Search::download_client (this=0x9706870, target=
  {static npos = 4294967295, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb48bf32c "#A#b#\214#x#\th#213#"}}, cid=
  {static npos = 4294967295, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb48bf328 "<#\t#A#b#\214#x#\th#213#"}}, filename=
  {static npos = 4294967295, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb48bf324 "<\201{\t<#\t#A#b#\214#x#\th#213#"}}, size=4696548889, tth=
  {static npos = 4294967295, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb48bf320 "#~{\t<\201{\t<#\t#A#b#\214#x#\th#213#"}}, hubUrl=
  {static npos = 4294967295, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0xb48bf31c "\204#q\t#~{\t<\201{\t<#\t#A#b#\214#x#\th#213#"}}) at linux/search.cc:1632
#12 0x081117ad in Func6<Search, std::string, std::string, std::string, long long, std::string, std::string>::call (this=0x97608b0)
at linux/func.hh:228
#13 0x08183b00 in WulforManager::processClientQueue (this=0x95009d8) at linux/wulformanager.cc:220
#14 0x08183bb9 in WulforManager::threadFunc_client (data=0x95009d8) at linux/wulformanager.cc:160
#15 0xb785e33f in ?? () from /usr/lib/libglib-2.0.so.0
#16 0x095009d8 in ?? ()
#17 0x094ff678 in ?? ()
#18 0x00000000 in ?? ()

Now, should we check the return value of that userPtr that is passed to QueueManager::add? By checking if the user.get() != 0. But it should still be possible to add result even if the original source has left (I guess there isn't and QM:add without userPtr?). Or should we store the UserPtrs (or SearchResultPtrs?) while the result list is open? (if I remember correctly dc++ stored the SearchResults in the tab while they were being displayed).

--RZ

[Steven Sheehy (steven-sheehy)]

I say we just check if ClientManager::getInstance()->findUser returns NULL or not. That should be sufficient. Previously we kept the SearchResultPtr in the UI and it contains a reference to UserPtr, so reference counting ensured that they were both still around when we passed them to the core. Now that we're using find we no longer can rely on them still existing and need to validate the results of findUser.

[Razzloss (razzloss)]

Then what do you suggest we pass to the QueueManager:add, which expects const UserPtr& as its parameter? Obviously passing the null pointer won't work, and even if it did the best case scenario would be that the download is added to queue without sources and it wouldn't start downloading when that user returns, without a new search.

--RZ

[Razzloss (razzloss)]

Yeah, that prevents the crash, but leaves unusable search results in the list. And even if removing results from users that left the hub was a practical option, It wouldn't be a good one. I'd say we'd need to restore the old behavior somehow (where one could queue "old" results). Though I think there's no other way to achieve that than saving the SearchResultPtrs (or UserPtrs) within the Search-tab, while the results are being displayed.

--RZ

[Steven Sheehy (steven-sheehy)]

What about just putting a status message in the status bar to let the user know that the user left the hub and that they can't use that search result? Kind of like how we notify a user left the hub in private messages. Is it really worth the effort to maintain a third copy of the search results (core, treeview, and your proposal)?

[Olorin (vivliofika)]

why the user can not use the search result??? It has TTH, filename, file size and user nick. So you can add the result to the queue but then you should check the other results to match. If no matches you should start the TTH search to get all matches will be found.

[Razzloss (razzloss)]

Olorin: Because currently the internal representation of user, which is needed for queueing isn't there anymore.

This discussion is now about, if its proper to leave those unusable results in there and when user tries to download them gets "HAAHAA! You can't download that anymore (with Nelson voice)" message in the statusbar.

Which I think is too annoying. I don't usually care where do I download anything as long as I get Linux isos queued with TTH, either the user comes back or someone else has the files. (plus I have a bad habit of leaving the search tabs open for loooong time and queue additional files when needed. I was quite suprised that it took me this long to notice the bug considering this habit, but apparently those people won't shutdown their computers either).

--RZ

..and this damn thing is broken with Konqueror and IE..

[Olorin (vivliofika)]

My uptime is about 2 months. LDCpp worktime is about 1 week. I have no problem with such downloads. But I don't want the client get fall if I click such search result. But I want to have such results in the list because of my usage: I do to the WAN HUB, search the film, then disconnect the WAN HUB and then I click the search result. If the client will crash, I'll cry then... Cry and cry and then copy the magnet. But Your client doesn't get the magnets from outspace. It has no any magnet handler. And the client doesn't do such thing as #linuxdc magnet:??????????? ===> search tub opens. So it's difficult to use offline search results.

[Steven Sheehy (steven-sheehy)]

Olorin: Dry your tears...we definitely won't leave any crashing bugs in linuxdcpp.

Razzloss: In that case, I'm okay with either a map or vector of UserPtr or SearchResultPtr. If you work on it (cause I won't be :D), we need to take a look at all the _client functions to see if we can use this new data structure.

[Razzloss (razzloss)]

Yeah, I kind of expected that this will be my problem from the start. I'll think somekind of map would be the best option. If done properly it would speed up the duplicate checking/adding results, which can get a bit cpu intensitive specially with older machines.

Should that patch of yours be committed as a temporary solution (with an added statusbar message)? It would prevent the crashes, while I work on the other solution (which might take a while judging by my calendar for the next 2 weeks, or maybe I should switch off my phone and never boot up the company laptop?)

--RZ

[Razzloss (razzloss)]

Committed changes adding unordered_map<string (cid), vector<SearchResultPtr> > and changes to addResult_gui. Didn't touch any of the _client (except parseSearchResult_client which is now _gui. Though I don't think it matters where this is called as long as the ResultPtr is available) functions yet (and probably won't as using this would require some locking mechanism). And most of the _gui callbacks deal with iterators anyway from which they parse the needed data together, so don't think any additional changes to _gui or _client functions are worth it.

--RZ

[Steven Sheehy (steven-sheehy)]

Why is SearchResultPtr used within gui functions? Would it not be better to make SearchResultPtrs and the result map be used only within client functions? This would save having to create and dispatch gui funcs for each of the duplicate results. Plus we should use this new data structure for more than just storage. Instead of calling ClientManager::getInstance()->findUser(cid) we could use this data structure to speed it up (findUser locks the ClientManager critical section).

[Olorin (vivliofika)]

YES!!! That is the cause of program freezing!!! Great work, Steven!

[Razzloss (razzloss)]

aww, what did Steven do? He was against the whole idea.

Anyway back to the subject. Of course you're right, if we moved the duplicate checking to ::on function results map wouldn't need to be accessed from the _gui and it could be used within the other _client functions. Though I'd still rather dispatch a SearchResultPtr instead of StringMap and convert the pointer to presentable strings in _gui (because ptr is just much lighter to pass around than the map (I don't know if this has that big of an effect on performance, I'll have to check)).

And at the same note, I don't think that changing the _client functions to use results map instead of findUser has that big of an effect on performance since the client functions are called from user actions and thus are quite rare (compared to addResult).

But that being said, I'll move the duplicate check to _client and remove the findUser calls. Depending on performance measurement results I'll change the StringMap passing back also.

--RZ

[Razzloss (razzloss)]

Back to In Progress, so that I remember to do the agreed changes.

--RZ