Multi rtorrent instances support #549
Conversation
zawapete
changed the title
|
I've only just started to review, but I must say I'm really impressed @zawapete. Thanks for this — it looks like you found a relatively simple way to tackle this daunting task. I'll need some more time to think about this and review the code, but I think I'm mostly in favor of consuming multiple rtorrent instances, for most of the reasons stated in this comment. Maybe your PR already does this, but in the future, it would be great to support both multiple rtorrent instances and multiple users. This would pave the way for more advanced administration controls. I really like the idea of configuring rtorrent in the UI, even for single-user instances. I've been thinking about this for a while — we could (and should) add error handling to improve the entire configuration experience. |
|
@jfurrow you can setup different users to the same rtorrent instances, but the db will be separated. This means that data that has rtorrent instance as source are shared, but data that has db as source are isolated (like rss feed, that I presume it works now out of the box, but I haven't tested such scenario). |
|
Leaving socket empty or inputting "false" on initial user creation (localhost and 5001 for host and socket) causes the following Registering a user without a socket throws the following error and the npm debug contains the follwoing: With that said, I didn't manage to run it but I'm super curious |
|
@jvacek I tested with socket, probably and input validation problem in saving host/port data. I will test and fix. can you attach the content of user.db? (at least the part for rtorrent connection data) |
|
@zawapete sorry I just realised my error, I was filling in both the port and the socket. I now managed to connect to my instance fine, added a new user with a different port, everything seems to work as expected from my side. One thing that would be preventing me from rolling this out for the time being is that the consecutive users can create more users. An admin/user distinction would be very useful. One case would be that the First created user is admin, all other ones are users by default, allow admin to give other users admin prvilleges. On the user experience side, except for the permissions and some UI tweaks, I give this A+, five stars and a diploma rating. Does what it should. Here's a couple bugs I encountered while running this version of the pull. I'll keep updating the thread as I find more bugs, I'll try using this version fo flood concurrently with a couple of my users for a week or so. This one occured to me when I was going through the settings, however that does not seem to be realted to this crash. Further tests I would suggest to do is see how many users this can hold at the same time, in the db, side by side, and with the insanely large amount of torrents this could mean. Imagine a scenario where a company like Feral would just start using this. Question is if that's something that this software should go in the direction of. Who knows, if the license is changed accordingly to make for-commerce use by paid-license only, and creating an admin panel for companies who would roll this software out to thousands of users, @jfurrow could get a nice little side-hustle :P But that's a whole 'nother issue. After this is merged, I would like to see the issue of creating rtorrent instances from the server (maybe first offer a simple guide, help generating a .rtorrent.rc file, etc.) be explored a little further. Really happy with this however, this software has some serious potential. |
|
@jvacek , yes, I will improve UI experience in order to have proper configuration. anyway the configuration you have to setup doesn't differ from the one in for the second bug I never encountered, I did some research and it could be a problem with the node and npm versions you are using: according to
your debug log shows the following:
only the user db is shared, all the other dbs are isolated for the currently logged in users. rutorrent use json files as persistency layers, that's mostly like nedb, so unless nodejs has far worse performance than php I expect a similar outcome |
This must be an issue with my whole node version screwup, I'm running the entire thing as root, managing the node version using n, and that's about that. I'll need to do some serious maintenance on this soon. Otherwise I have not found any other bugs. |
|
if the PR get accepted, before merging I will add an automatic migration for existing installation to the new db/config structure |
|
Thanks for the PR @zawapete! I still have the same concerns I expressed in Slack with regard to the service instances. In this PR, we're creating new instances of the services on-the-fly in a number of places. In With Flood's current architecture, we'll need to do the same thing — every user will need their own isolated instances of each service, and those instances need to be created as soon as the server begins running. Whenever a service's method is called in the application, the callee will need to retrieve the service instance for the user that initiated the request. To manage these instances, I think it makes sense to create a module that instantiates and stores the services for each user when the server starts, and exposes some getter methods to retrieve the services instances. Something simple like this would be ideal: const torrentServices = new Map();
const notificationServices = new Map();
// etc
users.forEach(userId => {
torrentServices.set(userId, new TorrentService());
notificationServices.set(userId, new NotificationService());
// etc
});
module.exports = {
getTorrentService: userId => torrentServices.get(userId),
getNotificationService: userId => notificationServices.get(userId)
// etc
};After thinking through these things some more, I'm wondering if it would be better to create an instance of each service per rTorrent instance, rather than per user. So if two users share the same rTorrent instance, they'll both access the same service instances. What do you think? (I think I'm okay with deferring that work to a separate PR, because it probably requires differentiating between user-related services and client-related services.) |
|
@jfurrow I tried something like you suggested in a commit I deleted: having the instance instantiated at request level in a middleware. it ended up with some issues with duplicated notifications and I removed it |
|
Hello everyone, First of all, thank you @zawapete for this multi-users feature that I was needing to setup a private-shared seedbox platform. I just wanted to know if there was an update on the db issues that makes flood crash ? After a download is finished, the notification icon goes crazy (displays 50+ times the same information) then the server crashes : Did a quick digging, turns out the notifications db is continuously updated with the same information (well, if I understand the error log correctly, "written over" would be more accurate than "updated") : So far, for a download that ended a few minutes ago I have more than 280 entries likes this, and still growing. At some point 2 write operations will concur (if I display the notification list in the GUI most probably ?) and the server will crash. I'm not good enough a developer to go deeper in the analysis, but there's definitely something that needs fixing here. Let me know though if I can be of any help Thank you |
|
@bananabomb59, thank you for giving my branch a try. as for the multiple notification, that could be a bug in my branch, even if I wasn't able to reproduce. can you tell me the commit you're testing with? |
|
1f5fda3 apparently (this is my first use of git, I've stopped developing a while ago ^^) I'll update to the latest commit to see if I still have the issue. |
|
OK updated to latest commit, I don't seem to have the multiple notifications issue. The notification behavior is still a bit erratic (sometimes the notification is triggered when the download is complete, sometimes it's not), but it's far less problematic. What's more problematic though is that twice now since I've started testing this commit, the rtorrent instance associated to one of my users has crashed. It seems to have happened when deleting a torrent and its data in the GUI, but it's not happening every time I do that. I'll give it some more tries later today, to see if I can identify a pattern to reproduce the issue. |
I encountered this problem as well: in order to trigger the notification the original code check the previous download percentage of the torrent and the current one: if the previous is |
|
Yes I figured it'd be something like this. Not a biggie then. About my other issue with rtorrent crashing twice earlier, I've set rtorrent in extra-verbose mode and restarded it, but by the inevitable Murphy's law it hasn't crashed again in the numerous tests of downloads and deletions I've made for the last hour. I don't believe in coincidence so I'm still expecting the anomaly to happen again, but can't say for sure when and how. I'll let you know when it does. |
server/services/servicesHandler.js
Outdated
| if (!notificationServicesInterval.get(userId)) { | ||
| notificationServicesInterval.set(userId, | ||
| setInterval(() => notificationServices.delete(userId), servicesPurgeInterval) | ||
| ); |
There was a problem hiding this comment.
What's the reason for purging this every 10 minutes?
|
@zawapete Can you explain more about the database issue? If I understand it correctly, you're loading multiple instances of the same database — presumably once per user. Is that correct? If so, it would be better if Flood loaded a single instance for the single database, right? Overall this is working pretty well for me, so far. I'd like to hear more about the database issue first and see if we can find a good solution, but I'm inclined to accept this PR into a feature branch. I'd like to work a bit more on this experience before merging into |
|
Some bugs I've noticed which should be fixed before merging into the feature branch (I may find more later, just FYI):
User A: User B: |
I was loading concurrently the same database, and nedb has an issue with it (there's a PR to introduce a locking mechanism but the project seems to be stale). basically when you load a nedb database the library creates a file with
we don't want to have services instances in memory when the user is not logged in: we can purge on logout but that's not reliable, since the user could just close the browser without logging out. I put a in place this purging time every 10 minutes to improve the memory footprint basically. in case the user is still using flood after that times (or he comes back), the services are instantiated seamless again I would check in the mentioned bugs. also before merging I would like to add an |
|
@zawapete This seems to be working really well! Thanks so much for your contributions and patience. I think an I don't really like the 10 minute purging, but let's go with it for now and see how it works. |
|
@jfurrow the PR is finished on my side. last thing missing is a migration to new db structure for existing installations: I don't know if we want to add it (maybe as a separate npm target?) |
|
@zawapete Awesome, I'll merge this into the feature branch and start testing it locally. Thanks for all your work! |
* Handling multiple rtorrent instances * performance issue addressing * fix wrong conflict resolution * performances * purge services instance every 10 mins * last fixes * avoid duplicated reducers
* Handling multiple rtorrent instances * performance issue addressing * fix wrong conflict resolution * performances * purge services instance every 10 mins * last fixes * avoid duplicated reducers
* Handling multiple rtorrent instances * performance issue addressing * fix wrong conflict resolution * performances * purge services instance every 10 mins * last fixes * avoid duplicated reducers
* Multi rtorrent instances support (#549) * Handling multiple rtorrent instances * performance issue addressing * fix wrong conflict resolution * performances * purge services instance every 10 mins * last fixes * avoid duplicated reducers * Adjusts UI of rtorrent connection form fields (#639) * Multi rtorrent instance cleanup (#676) * Cleans up multi-user logic. * Rename clientRequestService to clientGatewayService * [multi-user] Adds prompt to fix rtorrent connection failure (#681) * Adds ability to update user * Add connection test endpoints and client actions * Listen to client connectivity change events * Separates scgiUtil from clientRequestManager * Adds BaseService * Services extend BaseService * Listen for connetion change events in client * Reorganizes app structure * Adds prompt to correct per-user client interruption * Fixes linting errors * Removes debugging stuff * Fixes Size rendering * Fixes moveTorrents * Adds migration step for legacy database entries (#686) * Use FormattedMessage * Updates documentation * Fixes migration script for new installs * Updates dependencies * Moves string to i18n * Updates deps
Description
Related Issue
#460 (comment)
Motivation and Context
See issue
How Has This Been Tested?
Tested on a server with three separated rtorrent instances on socket
Types of changes
Once it works we could provide a migration script so that the change won't be breaking
Checklist: