boxo icon indicating copy to clipboard operation
boxo copied to clipboard

feat(session): do not record erroneous session want sends

Open hannahhoward opened this issue 2 years ago • 3 comments

Goals

Find acceptable fix for #432

Implementation

The basic idea here is there's nothing wrong with the connection event manager -- the problem lies in the fact that the PeerManager never tells the caller it didn't actually send anything cause the peer appeared disconnected. The SessionWantSender in turn updates the state of sent wants based on incorrect assumption something actually happened. This PR deals with the problem by helping the SessionWantSender avoid getting into an incorrect state by not updating its state when SendWants does nothing.

The subsequent connect event should trigger the sending of the WantBlock then.

For discussion

This is just an experiment and a suggestion for someone else to pick up, to help solve #432 more quickly -- I'm not doing the leg work of testing and change log etc

hannahhoward avatar Aug 23 '23 22:08 hannahhoward

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 60.44%. Comparing base (5965637) to head (2d6338d). Report is 1 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
+ Coverage   60.42%   60.44%   +0.01%     
==========================================
  Files         245      245              
  Lines       31056    31064       +8     
==========================================
+ Hits        18766    18777      +11     
+ Misses      10621    10616       -5     
- Partials     1669     1671       +2     
Files with missing lines Coverage Δ
bitswap/client/internal/peermanager/peermanager.go 91.85% <100.00%> (+0.12%) :arrow_up:
bitswap/client/internal/session/session.go 90.53% <ø> (ø)
...tswap/client/internal/session/sessionwantsender.go 96.03% <100.00%> (+0.05%) :arrow_up:

... and 9 files with indirect coverage changes

codecov[bot] avatar Aug 23 '23 22:08 codecov[bot]

seems reasonable, aside from probably needing some form of cleanup for cases where the error doesn't ever resolve - we don't want to continue to try this in perpetuity, unless there is already an alternative cleanup mechanism in place for these wants

rvagg avatar Aug 24 '23 00:08 rvagg

Presumably, the case where the error does not resolve is one where a peer has disconnected, in which case, we will eventually get a disconnected message.

hannahhoward avatar Aug 24 '23 00:08 hannahhoward