Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid handle when there is no torrent metadata #7415

Merged
merged 2 commits into from May 11, 2023

Conversation

xoriole
Copy link
Contributor

@xoriole xoriole commented May 9, 2023

The PR updates the require_handle decorator to execute the function if TorrentDef has metadata.

@xoriole xoriole marked this pull request as ready for review May 9, 2023 14:00
@xoriole xoriole requested a review from a team as a code owner May 9, 2023 14:00
@xoriole xoriole requested review from kozlovsky and removed request for a team May 9, 2023 14:00
and not result_future.done() \
and handle == download.handle \
and handle.is_valid() \
and not isinstance(download.tdef, torrentdef.TorrentDefNoMetainfo):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From issues, I see that the error arises in apply_ip_filter and force_dht_announce. But the fix is applied to all the @require_handle-decorated Download methods. The proposed fix suppresses the call of the corresponding handle method when the download with only an infohash is just started.

The description of TorrentDefNoMetainfo says:

Instances of this class are used when working with a torrent def that contains no metainfo (YET), for instance,
when starting a download with only an infohash

That is, in my understanding, having no meta info is just a temporal state of some torrent.

With this, I have two questions:

  1. Is it possible that for some other Download methods decorated with @require_handle, it is valid to have TorrentDefNoMetainfo as download.tdef and still call the corresponding handle method? Can the current PR accidentally break these other @require_handle-decorated methods when the downloading of a torrent with only an infohash is just started?

  2. The proposed fix skips calling the corresponding handle method. Is it safe? Is it possible that instead of just skipping, we should postpone calling these handle methods for apply_ip_filter etc., until the download is fully started and the handle is ready?

Copy link
Contributor Author

@xoriole xoriole May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For all methods where we require a valid handle, it is expected the torrent already has a metainfo indicating that it is a valid download. The case where there is no metainfo as mentioned in the docs is while doing a metainfo request to show the torrent details on the download dialog. At that point, the torrent is not a valid download where the methods requiring handle could be applied. In such a state, we shouldn't need to call any methods that should require a valid handle.

  2. I think it is safe to skip for the same reason above. For the methods which explicitly require a valid handle, there should be a valid handle and metainfo already present. For methods like apply_ip_filter and force_dht_announce, it is expected that the handle is valid, otherwise, trying to execute such action on download is not meaningful.

@kozlovsky kozlovsky self-requested a review May 11, 2023 10:13
@xoriole xoriole merged commit 339d27a into Tribler:release/7.13 May 11, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants