Provide torrent creation web API #19498
Conversation
|
Hi team. I would like to know your opinion about this change. I'm aware that the commit is not of a mergeable quality yet, but I would like to get opinions before spending too much time on it. Unfortunately, my C++ and Qt knowledge is quite rudimentary. |
|
@rcarpa I would strongly recommend that you divide the work into several atomic parts (within separate PRs):
The main claims to the currently suggested API (which caught the eye with a quick review):
You shouldn't touch any of .ts files. |
|
Also please read https://github.com/qbittorrent/qBittorrent/blob/master/CODING_GUIDELINES.me and try to follow it as accurately as possible. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
I've reworked the code to create a new controller for the task. I'm not sure about the name |
It was requested to provide separate Pull Requests in order to simplify reviewing. When the one containing torrent creator changes is approved and merged we can start reviewing the next one that provides WebAPI. |
|
Sorry, I misunderstood that. Please refer to #19500 |
| @@ -204,14 +205,22 @@ void TorrentCreator::run() | |||
| entry["info"]["source"] = m_params.source.toStdString(); | |||
|
|
|||
| checkInterruptionRequested(); | |||
| if (!m_params.savePath.isEmpty()) | |||
There was a problem hiding this comment.
Is it intended to allow empty save path?
There was a problem hiding this comment.
Yes, whoever creates a torrent via the API doesn't necessarily want the torrent on the disk. This allows the workflow "create a torrent" -> "retrieve the torrent via api" -> "push the torrent via api some other servers" -> "let the transfer happen between all servers". In this use-case, temporary .torrent files are just an additional un-needed hassle to manage.
The current PR came from my desire to implement Bittorrent as an additional wire synchronization protocol between storage elements in rucio
There was a problem hiding this comment.
In reality, the workflow will be more complicated. The workflow which I described will only used for files for which we don't (yet) know the bittorrent v2 merkle root and piece layers. But this avoids having to implement a custom agent to compute this data and communicate it to Rucio if the bittorrent client already runs on the storage server anyway. I proposed a related change to deluge too (deluge-torrent/deluge#430)
There was a problem hiding this comment.
So I believe it isn't needed to create torrent file if client need the data?
I would omit additional parameter and use only savePath to determine whether file or data to produce.
There was a problem hiding this comment.
In this case, if the client will try to fetch the file via the getFile action, it will open the file on disk? And fail if the file was removed ?
There was a problem hiding this comment.
I chose to use create/status/result(singular)/delete
👍
It feels somehow strange to have a "start" and "stop" action for torrent creation.
I have never insisted on exact copying. The main thing is to catch the essence.
I added a separate TorrentCreationManager singleton.
👎
Well, I don't even want to look there. I believe in advance that this is unnecessary. (I don't want to sound arrogant, but my experience says so.)
Why do you need yet another manager? Isn't the controller itself enough for you to have "managing" logic there?
There was a problem hiding this comment.
😭 this was a compete miss-understanding then, I thought that's what you were telling me to do in answer to
As for torrent creation jobs. You are correct. The jobs are bound to an api/session and they should get destroyed when the api/session is closed, but by default (I believe so?) bittorent keeps the session indefinitely. Do you have a suggested solution? I was thinking initially to keep track of jobs in the base/bittorrent/session singleton. But I'm not sure it's a good idea. Any suggestions?
When you said to check the SearchController. By storing the torrent creation tasks in a singleton rather than inside the controller, they are not bound to a web session anymore. Having them managed by a web session doesn't allow to enforce any limits on the number of total torrent creation tasks allowed globally in parallel: the user can always bypass the limit by logging in parallel to a new Web Session and submitting new tasks under the new session.
That being said: I don't have any strong preferences for any of the two versions:
- I can rollback to the previous version, where everything was handled inside the controller, but had the limitations of impossibility to enforce limits + the limitation that creation tasks where not visible across 2 different WebSession (if we ever add the functionality to the web ui, the users will be surprised if the torrent creation jobs disappear after a disconnect).
- I can keep the new version with the Manager which has some advantages, but introduces a new singleton. This seems to be non-desired according to your last comment.
There was a problem hiding this comment.
@rcarpa
As I said before I still didn't deal with it carefully so I can miss some important details.
Could you first provide some general description of how you think it should be designed?
There was a problem hiding this comment.
@glassez , for my own requirements, I just need a way to create a torrent file via the api. I really don't have any preferences concerning technical aspects of the implementation. I try to do my best to implement things in a way similar to what already exists and to follow your pointers to achieve a better-quality result, to fulfill the needs of the broader community of this project. However, it's my first contribution to the project and I don't have near enough high-level perspective to design it in a way which fits the long term vision of the project. It will be very useful if somebody with a long experience in the project will give it a thought and will tell what's desired and what's not desired.
There was a problem hiding this comment.
@rcarpa
Hmm... it seems that there are some aspects of this case that I didn't realize at first. I need to think about it. Then I will try to summarize some of my judgments so that we can continue the discussion more productively.
rcarpa
changed the title
glassez
changed the title
|
IMO, the main problem is that the handling of torrent creation tasks does not fit with "regular" use case of qBittorrent Web API, especially because of the fact that creating a torrent is a time-consuming process with an indefinite duration, which does not go well with the session-based model we use to access API. It may have several solutions, and it looks like they are all imperfect (maybe someone else will offer a better solution?).
However, we should still have a global limit on the number of threads running simultaneously.
Both options above are not suitable for the use case when the user just wants to start creating a torrent, but does not control its process and result, at least by means of the API itself. (I don't know how possible this use case is.) However, the second option can be expanded with a parameter specifying that the "task" should be automatically deleted after completion. What do you think? P.S. Could someone tell about any other aspects that need to be taken into account when solving this problem? |
|
I just rebased the PR. No other changes. Concerning your analysis, to me it sounds like (1) is the version before the last push; before introducing the Manager. While (2) is the current version. As I have both versions almost finalized, I don't have any particular preferences. It's your's and @Chocobo1 call to say what's better. |
|
Note that I didn't have enough free time to investigate it thoroughly, just some basic ideas.
I would chose this.
I'm not sure if 'task' concept is still required after the torrent creation ends. I would imagine if a user wants to check the results he can just look at the log (assuming we logged the result). The 'task' is still useful when the torrent creation is running which let the user to cancel/stop the operation. |
Edit: I assume you suggest to directly add all created torrents to the session? While this is a possibility, I'm, for example, interested in the use-case when a torrent file is created without adding it to the session. |
👍
At first glance, this is not so trivial for an pure WebAPI (not WebUI) user. |
Personally, I am interested in general questions. I could help with the details myself. |
Currently there is no mechanism to send generic files or at least it wouldn't be a positive user experience with the current architecture. So for now I won't consider it. Generally speaking, if a web user only wants to create a .torrent file, there are existing browser side implementation: https://kimbatt.github.io/torrent-creator/ and I don't mind seeing it embedded into qbt. |
I'm starting to lean towards the same opinion. Indeed, what could be the purpose of creating a torrent file for existing data if they are not going to seed it? |
|
@Chocobo1 |
|
I have an use-case when the ability to create the torrent on the remote server without immediately adding it to the session will come in handy. This is the use-case which triggered my work on this particular PR. I'm a developer for a data-management application(rucio. It its essence, it's a catalog of many large files. These files are distributed on hundreds of storage servers around the globe. Files have to be regularly moved between storage servers. For example, to ensure redundancy; or data locality: move multiple related files close to a compute cluster to perform a data analysis on these files. Files are generated once, but moved many times. They are frequently quite big: 1GB+. I'm willing to add BitTorrent support as one of possible ways to execute the transfers between (a subset of) servers. The idea is to have a BitTorrent agent (ie: qBittorrent) run on each server which desires to use this protocol for data transfer. I'm restricting myself to only using bittorent v2. Files are transferred frequently, so generation of the merkle sha256 tree and
I agree that it's possible to use a third-party tool for that. But this means having to add an additional tool. And setting up one more communication channel with the storage server to retrieve the output of that tool. At the same time, qBittorrent is already running on the server and is equipped with everything needed to build the torrent and allow me to fetch it. That being said, I don't want to push my use-case onto the whole qBittorrent community if it's judged way too specific to serve anybody's else's needs. But it gives one possible answer to :
|
So, considering that we can make "torrent creation task" objects quite lightweight, we can still store a certain number of completed "tasks", having limited them with something like a circular buffer. |
So we could not make the function of receiving files to be general purpose, but limit it only to the possibility of receiving files created within some of "torrent creation task". |
|
Actually, there is already a function which does a similar action . It fetches the torrent from the session and exports it. (I plan to use this work-around if my try to support creation without adding to session is not accepted 😁 ) qBittorrent/src/webui/api/torrentscontroller.cpp Line 1432 in c805606 How will an action to fetch the result of a creation task be different? |
rcarpa
force-pushed
the
master
branch
2 times, most recently
from
7778669
to
b906ba5
Compare
|
I implemented a limit on the maximum number of tasks. For that, I relied on boost::multi_index as a container. I hope this isn't problematic. I saw that boost is already used by the project, but I don't know how OK it is to use all its functionalities. Thanks to it, tasks can be accessed by two indexes: either by the task id; or by the completion status and date. This avoids the need for maintaining two separate data structures and synchronize state between them. I slightly deviated from the agreement above. Instead of limiting the number of completed tasks, I limited the total number of tasks (including incomplete). However, when the limit is reached, but a new task is submitted, the oldest completed task will be automatically removed. If all tasks are incomplete, an error is returned to the user. This is because I wasn't able to find a way to implement a limit on completed tasks without keeping track of them in a separate data structure (or, alternatively, iterating over all tasks each time in O(n)). If you have better ideas on how to achieve the goal, I'd be happy to learn about them. |
|
bumping for progress |
|
Testers needed here |
|
I'll take a look at this when I get a chance but work has me buried up to my neck. Looks promising. |
|
This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity. |
|
bumping |
src/webui/api/metafilecontroller.h
Outdated
| @@ -0,0 +1,50 @@ | |||
| /* | |||
| * Bittorrent Client using Qt and libtorrent. | |||
| * Copyright (C) 2018 Thomas Piccirello <[email protected]> | |||
There was a problem hiding this comment.
@rcarpa
Your copyright should be here. And in those files that you have changed, your copyright should be added to the existing ones.
There was a problem hiding this comment.
I pushed them in a new commit. Feel free to squash it into the original one when you'll be working on finalizing the PR.
I see however that these copyrights aren't quite kept up to date. Aren't they just duplicating information which is available in the git history anyway, while introducing risks of merge conflicts?
Implement a new api controller to handle generation of .torrent files. Add a new TorrentCreationManager singleton which keeps tracks of torrent creation tasks. Use a separate ThreadPool in the Manager. It acts as a queueing mechanism for creation tasks to avoid too many tasks running in parallel. Slightly adapt the TorrentCreator for the new needs: allow to return the content of the generated .torrent file without requiring to write it to disk at all. If no savePath is passed, the created torrent file will be kept in memory. Also, communicate the actual pieceSize used for generation (if it's set to 0, libtorrent selects it automatically). By default, the created torrents will be added to the session. This behavior can be disabled by setting `startSeeding = false`. The maximum number of tasks in the manager is bounded by a configuration value. If this limit is reached, the manager will automatically remove the oldest completed task. If no such task exists (all tasks are pending or active), an error will be returned to the user. Closes qbittorrent#5614.
rcarpa
force-pushed
the
master
branch
2 times, most recently
from
69e15b8
to
ee71824
Compare
|
Superseded by #20366. |
Implement a new api controller to handle generation of .torrent files.
Add a new TorrentCreationManager singleton which keeps tracks of
torrent creation tasks. Use a separate ThreadPool in the Manager.
It acts as a queueing mechanism for creation tasks to avoid too many
tasks running in parallel.
Slightly adapt the TorrentCreator for the new needs: allow to return
the content of the generated .torrent file without requiring to write
it to disk at all. If no savePath is passed, the created torrent file
will be kept in memory. Also, communicate the actual pieceSize used for
generation (if it's set to 0, libtorrent selects it automatically).
Closes #5614.