Always count unhandled requests as pending

Fixes https://github.com/anacrolix/torrent/issues/679.

diff --git a/peerconn.go b/peerconn.go
index e17d211680bb7a1e8aa0f19bf654b26326ac1b1f..8e1f654d7ad022327eda0db3e859dadcb2da70e6 100644 (file)
--- a/peerconn.go
+++ b/peerconn.go
@@ -1076,15 +1076,16 @@ func (c *PeerConn) mainReadLoop() (err error) {
                        if !c.fastEnabled() {
                                c.deleteAllRequests()
                        } else {
-                               c.actualRequestState.Requests.Iterate(func(x uint32) bool {
-                                       if !c.peerAllowedFast.Contains(x / c.t.chunksPerRegularPiece()) {
-                                               c.t.pendingRequests.Dec(x)
-                                       }
-                                       return true
-                               })
+                               // We don't decrement pending requests here, let's wait for the peer to either
+                               // reject or satisfy the outstanding requests. Additionally some peers may unchoke
+                               // us and resume where they left off, we don't want to have piled on to those chunks
+                               // in the meanwhile. I think a peers ability to abuse this should be limited: they
+                               // could let us request a lot of stuff, then choke us and never reject, but they're
+                               // only a single peer, our chunk balancing should smooth over this abuse.
                        }
                        c.peerChoking = true
-                       // We can then reset our interest.
+                       // We can now reset our interest. I think we do this after setting the flag in case the
+                       // peerImpl updates synchronously (webseeds?).
                        c.updateRequests("choked")
                        c.updateExpectingChunks()
                case pp.Unchoke:
@@ -1099,7 +1100,6 @@ func (c *PeerConn) mainReadLoop() (err error) {
                        c.actualRequestState.Requests.Iterate(func(x uint32) bool {
                                if !c.peerAllowedFast.Contains(x / c.t.chunksPerRegularPiece()) {
                                        preservedCount++
-                                       c.t.pendingRequests.Inc(x)
                                }
                                return true
                        })
@@ -1169,24 +1169,6 @@ func (c *PeerConn) mainReadLoop() (err error) {
                case pp.AllowedFast:
                        torrent.Add("allowed fasts received", 1)
                        log.Fmsg("peer allowed fast: %d", msg.Index).AddValues(c).SetLevel(log.Debug).Log(c.t.logger)
-                       pieceIndex := msg.Index.Int()
-                       // If we have outstanding requests that aren't currently counted toward the combined
-                       // outstanding request count, increment them.
-                       if c.peerAllowedFast.CheckedAdd(msg.Index.Uint32()) && c.peerChoking &&
-                               // The check here could be against having the info, but really what we need to know
-                               // is if there are any existing requests.
-                               !c.actualRequestState.Requests.IsEmpty() {
-
-                               i := c.actualRequestState.Requests.Iterator()
-                               i.AdvanceIfNeeded(t.pieceRequestIndexOffset(pieceIndex))
-                               for i.HasNext() {
-                                       r := i.Next()
-                                       if r >= t.pieceRequestIndexOffset(pieceIndex+1) {
-                                               break
-                                       }
-                                       c.t.pendingRequests.Inc(r)
-                               }
-                       }
                        c.updateRequests("PeerConn.mainReadLoop allowed fast")
                case pp.Extended:
                        err = c.onReadExtendedMsg(msg.ExtendedID, msg.ExtendedPayload)
@@ -1534,9 +1516,7 @@ func (c *Peer) deleteRequest(r RequestIndex) bool {
                f(PeerRequestEvent{c, c.t.requestIndexToRequest(r)})
        }
        c.updateExpectingChunks()
-       if !c.peerChoking || c.peerAllowedFast.Contains(r/c.t.chunksPerRegularPiece()) {
-               c.t.pendingRequests.Dec(r)
-       }
+       c.t.pendingRequests.Dec(r)
        return true
 }
 

diff --git a/requesting.go b/requesting.go
index c1ae8adac57ede1aaecabc4f426584a44d6edcbb..74d1d682e317261b7fc6b5d9b483d14071a1d836 100644 (file)
--- a/requesting.go
+++ b/requesting.go
@@ -144,9 +144,8 @@ func (p *peerRequests) Less(i, j int) bool {
                if current {
                        ret--
                }
-               // I have a hunch that this could trigger for requests for chunks that are choked and not
-               // allowed fast, since the current conn shouldn't already be included. It's a very specific
-               // circumstance, and if it triggers I will fix it.
+               // See https://github.com/anacrolix/torrent/issues/679 for possible issues. This should be
+               // resolved.
                if ret < 0 {
                        panic(ret)
                }