Allow qBittorrent to determine hash ahead of time to avoid association errors#610
Allow qBittorrent to determine hash ahead of time to avoid association errors#610T4g1 wants to merge 3 commits into
Conversation
therobbiedavis
left a comment
therobbiedavis
left a comment
There was a problem hiding this comment.
Looks good for the most part. We need to clean up dead code, and I had a question about BencodeNET
| if (!loginResponse.IsSuccessStatusCode) | ||
| { | ||
| var body = await loginResponse.Content.ReadAsStringAsync(cancellationToken); | ||
| var redacted = LogRedaction.RedactText(body, LogRedaction.GetSensitiveValuesFromEnvironment().Concat([client.Password ?? string.Empty])); |
There was a problem hiding this comment.
This doesn't look be used
There was a problem hiding this comment.
I think there might be some more dead code in the adapter like TryPredownloadTorrentFileAsync(), but it's not a blocker. If you get a chance, we should probably also try to clean up the files as we touch them.
There was a problem hiding this comment.
I agree but as I already have a whole PR dedicated to cleaning the adapters (#599) i'd rather not change too much here
therobbiedavis
left a comment
therobbiedavis
left a comment
There was a problem hiding this comment.
Great job, I still have some hesitation around the cookies and url only torrent adds but I think this is looking better.
Sidenote: Let's make sure we don't commit an impermissible torrent to the repo 😆
|
|
||
| // Register named HttpClients for each adapter type so adapter implementations can request the appropriately-configured client. | ||
| // qbittorrent | ||
| builder.Services.AddSingleton<CookieContainer>(); |
There was a problem hiding this comment.
My suspicion is that this could cause errors for those running multiple instances under the same host, like a docker user would. What about moving this into a qBittorrent session manager keyed client.Id + normalized baseUrl + username?
| } | ||
| } | ||
| else | ||
| byte[]? torrentFileData = result.TorrentFileContent; |
There was a problem hiding this comment.
I think this regresses URL-only torrent adds. If pre-download returns Empty, we still have a valid httpTorrentUrl, but hash remains empty and AddAsync returns null before reaching the URL add branch. qBittorrent supports adding http/https torrent URLs directly, and canary handled this by adding the URL and then detecting the new hash from torrents/info afterward. I am not sure the best way to handle this with the new direction here though. wdyt?
There was a problem hiding this comment.
Indeed, if we can't get the torrent file, we assume qBittorrent will not be able to get it either, hence the branch to add with Magnet/URL will only perform the magnet add
Do you think we should handle the URL case even though we are unable to get the torrent ourselves ? In that case, we re-introduce the issue where we cannot garantee that the hash can be found afterwards
| if (!loginResponse.IsSuccessStatusCode) | ||
| { | ||
| var body = await loginResponse.Content.ReadAsStringAsync(cancellationToken); | ||
| var redacted = LogRedaction.RedactText(body, LogRedaction.GetSensitiveValuesFromEnvironment().Concat([client.Password ?? string.Empty])); |
There was a problem hiding this comment.
I think there might be some more dead code in the adapter like TryPredownloadTorrentFileAsync(), but it's not a blocker. If you get a chance, we should probably also try to clean up the files as we touch them.
2 participants
Summary
Follows two reports of association errors on Discord support channel
Changes
Changed
Testing
Notes
As i was not able to reproduce the issue discussed on Discord (association errors when adding torrents to qBittorrent), this might not solve the real issue but it should be an improvement nonetheless