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

Sorting the started view causes announces to fire #449

Closed
Speeddymon opened this issue Jul 1, 2016 · 16 comments
Closed

Sorting the started view causes announces to fire #449

Speeddymon opened this issue Jul 1, 2016 · 16 comments

Comments

@Speeddymon
Copy link
Contributor

Ref: #386

Sorting the started view, such as on a schedule, causes the announces for all started torrents to fire.

This can lead to announces being sent to trackers every second, in some schedules, which can lead to being banned from some trackers, or at the very least, a warning from the administrators about hammering the tracker.

The suspected cause is that the torrents are all removed from the started view, and then added back in the specified order, which causes the state of the torrents to change from 1 to 0 to 1 again, and that all runs through the following line of code:

"view.event_added = started,{(view.set_not_visible,stopped),(d.state.set,1),(scheduler.simple.added)}\n"

The state change is likely what is causing the announce itself to be sent, however the sorting of the view should ideally not trigger a state change, except that right now it does.

@Speeddymon
Copy link
Contributor Author

Speeddymon commented Jul 2, 2016

Hi,

I have some additional information to add which is sensitive in nature. Please provide me details on how I can provide this additional information privately to one of the developers.

@chros73
Copy link
Contributor

chros73 commented Jul 5, 2016

On second thought, I'm not sure whether the state change is responsible for this.
I'd vote for the scheduler.simple.added.
Can you try out with stopped view? This doesn't include the above command. I'd be curious if we reproduce the problem or not. (I can't reproduce the bug with stopped view.)

Edit: I misunderstood the concept late night :)
The interesting part is that only these 2 view has view.event_added and view.event_removed commands.

I tried out what happens if you override them in your config, e.g. like this:

view.event_added = started,{(d.state.set,1),(scheduler.simple.added)}
view.event_removed = started,{(view.set_visible,stopped),(scheduler.simple.removed)}
schedule2 = sort_started,12,10,"view.sort=started"

Note, that the 2nd line is the same as in main.cc, I only removed (view.set_not_visible,stopped) command from the 1st line, and this way there is no bug, "only" the started view gets empty. :)

@Speeddymon
Copy link
Contributor Author

I have a sort for the stopped view also. It doesn't cause this, however I figured that was due to the stopped torrents not being announced to the trackers.

Still need to relay some info to you privately somehow, for what it's worth.

@chros73
Copy link
Contributor

chros73 commented Jul 5, 2016

Thanks for the feedback: I edited the post above in the meantime (sorry, I haven't noticed that you've already answered).

@Speeddymon
Copy link
Contributor Author

Ok let me try your modified config and see if the other bug is present with it also.

@Speeddymon
Copy link
Contributor Author

view.sort_current=started,less=d.name=
view.sort_new=started,less=d.name=
view.sort=started
view.event_added = started,{(d.state.set,1),(scheduler.simple.added)}
view.event_removed = started,{(view.set_visible,stopped),(scheduler.simple.removed)}
schedule2=sort_started,10,10,view.sort=started

Unfortunately the above causes all torrents to stop every time I try to start them.

(Note: Since my OP, I went through and updated all commands in the config file to the 0.9.6 way of doing them)

@chros73
Copy link
Contributor

chros73 commented Jul 6, 2016

Yes, I noticed that.
I did dig into the source code and I think I've found the problem and probably the fix for it.
But first:

  1. It seems that these 2 views (started and stopped) are special https://github.com/rakshasa/rtorrent/blob/master/src/main.cc#L308
    They are like some entry points for managing torrents. That's kind of weird to handle something like this in a view.
    That means we can't modify the logic defined there (because I don't know how it works :) ).
  2. The problem is: actually not scheduling view.sort but view.filter !
    Add only this to your working config and we recreated the bug:
    schedule2 = filter_started,12,10,"view.filter = started, not=$d.complete="
    Because view.sort doesn't include anything special https://github.com/rakshasa/rtorrent/blob/master/src/core/view.cc#L248 , but below it view.filter does (!): it deals with the defined event commands in main.cc https://github.com/rakshasa/rtorrent/blob/master/src/main.cc#L308
    Though view.sort using filter first : https://github.com/rakshasa/rtorrent/blob/master/src/core/view_manager.cc#L92 , and that's our problem.
  3. Quick dirty solution: do NOT allow filtering for started and stopped views inside view.sort.
    See this patch (will be fine with 0.9.6 and current master as well): https://github.com/chros73/rtorrent-ps/blob/fix_sort_started/patches/ps-fix-sort-started-stopped-views_all.patch
    And here's the branch for it in patched rtorrent-ps (if you want to try it out): https://github.com/chros73/rtorrent-ps/tree/fix_sort_started
    After compiling and trying it out we shouldn't see any weird problem with scheduling view.sort on started and stopped views with:
    schedule2 = sort_started,12,20,((view.sort,started))
    This means that we sacrifice scheduled filtering capability (via sorting) for sorting capability. But I don't think that it's an issue for now, since they aren't filtered by default anyway. And with this patch we can fix a potential, silent and really critical bug.

@chros73
Copy link
Contributor

chros73 commented Jul 6, 2016

The logic does work (I made a silly bug in the condition, now it's fixed). I've updated the post above.
I don't experience the issue on my test setup with this patch: https://github.com/chros73/rtorrent-ps/blob/fix_sort_started/patches/ps-fix-sort-started-stopped-views_all.patch

To be clear, with this patch:

  • we CAN use scheduled view.sort command on these views: started , stopped
  • we are still NOT allowed to use view.filter command on these views: started , stopped

The latter has to be mentioned later on the wiki under Issues: https://github.com/rakshasa/rtorrent/wiki/Issues (I just linked this issue for now.)

@Speeddymon Can you try it out? And thanks for mentioning this and for contributing to the wiki as well!

@rakshasa If you agree with the logic above and you'll accept this dirty fix then I will create a pull request.

@chros73
Copy link
Contributor

chros73 commented Jul 6, 2016

Patch2 (instead of the previous one): https://github.com/chros73/rtorrent-ps/blob/fix_filter_started/patches/ps-fix-sort-started-stopped-views_all.patch
branch: https://github.com/chros73/rtorrent-ps/tree/fix_filter_started

  • since view.filter causes problem not view.sort, don't allow to use view.filter with started and stopped views.

It works better then the previous one:

  • no issue either
  • it ignores scheduled filtering like this: schedule2 = filter_started,12,10,"view.filter = started, not=$d.complete=" , so the bug won't be triggered by accident.

(I've searched through the source code and it shouldn't break anything.)

@Speeddymon Can you try out this patch2 (that patches view.cc)?
It works for me, e.g. with this rtorrent-ps config:

# started view
schedule2 = filter_started,12,10,"view.filter = started, not=$d.complete="
branch=pyro.extended=,"view.collapsed.toggle=started"
branch=pyro.extended=,"view.sort_current = started,\"compare=+-,d.complete=,d.last_active=\"","view.sort_current = started,greater=d.last_active="
schedule2 = sort_started,12,10,((view.sort,started))

# stopped view
schedule2 = filter_stopped,12,10,"view.filter = stopped, d.complete="
branch=pyro.extended=,"view.collapsed.toggle=stopped"
branch=pyro.extended=,"view.sort_current = stopped,\"compare=++-,d.complete=,d.throttle_name=,d.custom=tm_loaded\"","view.sort_current = stopped,greater=d.custom=tm_loaded"
schedule2 = sort_stopped,12,10,((view.sort,stopped))

@Speeddymon
Copy link
Contributor Author

Hi @chros73

Seems my last comment didn't get posted.

I can't compile on this machine as its not my machine to muck around with, even though I have root.

However, if @rakshasa accepts the fix, I can get the distro maintainers to port the fix to their build and I can yum update rtorrent with the new patch.

I'm hopeful it will also fix another bug I need to report privately but haven't yet been able to figure out how to do so.

@chros73
Copy link
Contributor

chros73 commented Jul 6, 2016

Ok, no problem. But it won't happen soon, I think :)

@Speeddymon
Copy link
Contributor Author

That's fine for me. The sorting is not a big issue so much since I mainly watch the Active view. :-) But, eventually will be nice to have for started view. :-) Thanks again for your help with diagnosing this.

@chros73
Copy link
Contributor

chros73 commented Jul 6, 2016

And fixing it :) Yw.
Btw, have you used rtorrent-ps?

@Speeddymon
Copy link
Contributor Author

And fixing it. Yes. :-)

No, I'm afraid not since I don't own the server.

@chros73
Copy link
Contributor

chros73 commented Jul 7, 2016

I've added and commented the pull request.

(About rtorrent-ps: try to convince them or you can compile it into your home dir. It will open up a whole new world! :) )

@Speeddymon
Copy link
Contributor Author

I have reported this downstream with Fedora, in the hopes that they will include the fix in their build, in spite of a lack of a response from @rakshasa

https://bugzilla.redhat.com/show_bug.cgi?id=1354125

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

No branches or pull requests

2 participants