Opened 10 years ago

Closed 10 years ago

#4526 closed Bug (fixed)

do not skip scrape unless all three scrape fields came with announce

Reported by: reardon Owned by: jordan
Priority: Normal Milestone: 2.50
Component: libtransmission Version: 2.33
Severity: Normal Keywords: backport-2.4x
Cc:

Description

the current logic in on_announce_done() will skip scrapes on a torrent if any of three scrape fields came back with the announce:

a. complete
b. incomplete
c. downloaded

In my experience, some trackers (actually, more than half of the private trackers I use) return 'complete' and 'incomplete' with announce but yield 'downloaded' only with scrapes.

Thus, the check should be a && b && c rather than a
b c.

Attachments (1)

keep-scraping.patch (4.6 KB) - added by reardon 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by reardon

(formatting)

Thus, the check should be

 a && b && c rather than a || b || c. 

Further, getting one of those three currently causes all three to be reset. Instead, each field should be updated only if the field was reported by the tracker.

comment:2 Changed 10 years ago by reardon

Ignore last patch (no-multiscrape) please, got it swapped around with the intended patch.

Changed 10 years ago by reardon

comment:3 Changed 10 years ago by reardon

Any thoughts on this? I really think r12605 got this wrong. The _majority_ of the trackers I touch are affected by this problem. It's actually worse in 2.40 than 2.33

It is compounded by treating "no value returned" as "zero". My patch addresses both.

Last edited 10 years ago by reardon (previous) (diff)

comment:4 Changed 10 years ago by jordan

  • Component changed from Transmission to libtransmission
  • Keywords backport-2.4x added
  • Milestone changed from None Set to 2.50
  • Owner set to jordan
  • Status changed from new to assigned

fixed in r12955.

Unfortunately, 2.40 is already out. I've committed this to trunk, and will backport to the 2.4x branch if/when the dev team decides to make another 2.4x release.

comment:5 Changed 10 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:6 Changed 10 years ago by reardon

r12955 still has two problems: it doesn't fix on_scrape_done() and it doesn't treat scrape-field-is-missing different than scrape-field-is-zero. As I explain above, previous scrape results will be reset to zero by on_announce_done() when those scrape fields were not returned during the announce. ie, response handlers in announcer-http.c know the difference between a field with a true zero result and a field that was not sent, whereas the handlers in announcer.c get a zero in both cases and cannot tell the difference. That is why my patch sets each result to -1 in announce-http if the field was not sent.

The problem exists in both on_announce_done() and on_scrape_done(), with each incorrectly resetting the scrape results of the other.

The resulting behavior: transmission will rotate between the real values and false values; after a scrape the values will be correct but only until the next announce, when they will be wrong again until the next scrape, and so on.

Unfortunately, real-world trackers do not follow any sort of proper logic when returning scrape results. Some trackers send a superset of results on scrape, some send a subset, some send part on announce and another part on scrape, etc. To handle this properly, transmission must distinguish between missing fields and zero fields.

comment:7 Changed 10 years ago by x190

Seems to me scrapes are an unnecessary part of bittorrent. What real benefit do they provide?

comment:8 Changed 10 years ago by reardon

x190: um, ok.

Devs: Since the bug is closed I'm just being a squeaky wheel and prompting for followup... which I know is coming ;) I spent a ton of time capturing tracker traffic to make sure I got this right and hope you agree.

comment:9 Changed 10 years ago by livings124

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:10 Changed 10 years ago by jordan

reardon, I agree with your discussion of field-is-zero vs field-is-missing, but rather than assigning them to -1 in an "else" conditional, let's initialize them to -1 when the response struct is allocated. That way we won't get a 0 value as a side-effect of not reaching the response parsing block of code.

The same issue exists in announcer-udp, so let's initialize the seeders/leechers/downloads fields to -1 there as well.

comment:11 Changed 10 years ago by jordan

done in r12982.

reardon: I think this completes this ticket, but I'd like you to review the changes before I mark it as closed.

comment:12 Changed 10 years ago by reardon

Works. Moving the initializers into tr_tracker_http_announce() and tr_tracker_http_scrape() is cleaner code.

Sorry I had a bozo set-to-zero mistake in on_announce_done()

One small thing: why does transmission even look for the "downloaders" response? I've never seen it, which is why my original patch ignores it, and it was never initialized trackerConstruct()

comment:13 Changed 10 years ago by jordan

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.