LaunchManyCore fails to produce notification on torrent completion

[ichorid]

If a download takes less than a second to complete, sesscb_states_callback fails to produce the notification on torrent finish.
It happens because new_active_downloads only gets new entries if their status is DLSTATUS_DOWNLOADING:

tribler/Tribler/Core/APIImplementation/LaunchManyCore.py

Lines 679 to 698 in 8981c83

	if state == DLSTATUS_DOWNLOADING:
	new_active_downloads.append(safename)
	elif state == DLSTATUS_STOPPED_ON_ERROR:
	self._logger.error("Error during download: %s", repr(ds.get_error()))
	if self.download_exists(tdef.get_infohash()):
	self.get_download(tdef.get_infohash()).stop()
	self.session.notifier.notify(NTFY_TORRENT, NTFY_ERROR, tdef.get_infohash(), repr(ds.get_error()))
	elif state == DLSTATUS_SEEDING:
	seeding_download_list.append({u'infohash': tdef.get_infohash(),
	u'download': download})
	if safename in self.previous_active_downloads:
	self.session.notifier.notify(NTFY_TORRENT, NTFY_FINISHED, tdef.get_infohash(), safename)
	do_checkpoint = True
	elif download.get_hops() == 0 and download.get_safe_seeding():
	# Re-add the download with anonymity enabled
	hops = self.session.config.get_default_number_hops()
	self.update_download_hops(download, hops)
	self.previous_active_downloads = new_active_downloads

This makes notification fail for corner cases, like unit tests.

[devos50]

Nice find! Certainly an edge case. I guess it's not hard to reproduce this with a unit test and fix it? 👍

[qstokkink]

Adding the download to self.previous_active_downloads in add() should fix this I think.

Also, I just noticed self.previous_active_downloads identifies torrents by name and not by infohash. That's quite a brittle design.

[ichorid]

After pondering on this for a few hours I can say that it is impossible to fix this problem correctly without refactoring the libtorrent wrapper.

[qstokkink]

Related to #3418

[qstokkink]

I printed the LibTorrent alerts:

CRITICAL 1531146077.61    LibtorrentDownloadImpl:500  (root)  state_changed_alert: ubuntukylin-17.10-desktop-amd64.iso: state changed to: downloading
CRITICAL 1531146077.61    LibtorrentDownloadImpl:500  (root)  torrent_checked_alert: ubuntukylin-17.10-desktop-amd64.iso checked
CRITICAL 1531146077.62    LibtorrentDownloadImpl:500  (root)  state_changed_alert: ubuntukylin-17.10-desktop-amd64.iso: state changed to: finished
CRITICAL 1531146077.62    LibtorrentDownloadImpl:500  (root)  torrent_finished_alert: ubuntukylin-17.10-desktop-amd64.iso torrent finished downloading
CRITICAL 1531146077.62    LibtorrentDownloadImpl:500  (root)  state_changed_alert: ubuntukylin-17.10-desktop-amd64.iso: state changed to: seeding

We need the first state change downloading/finished (state_changed_alert) after the checked alert (torrent_checked_alert).

[qstokkink]

This seems like it will be fixed in #3775, so I will mark this as completed with AllChannel2.0.

[ichorid]

@qstokkink, I used a different mechanism there, so it is not fixed. This will be fixed with redesign of Libtorrent wrapper, so it should stay opened as a reminder.

[devos50]

@ichorid we addressed this already with the finished callback in the LibtorrentDownloadImpl itself right?

[ichorid]

Solved by #4090

[ichorid]

In fact, this is only solved for GigaChannels, because we now don't rely on triggers, but instead schedule periodic channels DB consistency checks.