p2p: support zero length request protos #1958
Conversation
| @@ -223,10 +223,19 @@ func SendReceive(ctx context.Context, tcpNode host.Host, peerID peer.ID, | |||
| return errors.Wrap(err, "close write", z.Any("protocol", s.Protocol())) | |||
| } | |||
|
|
|||
| zeroResp := proto.Clone(resp) | |||
There was a problem hiding this comment.
what if resp is NOT a zero/empty response?
There was a problem hiding this comment.
true, that is an edge-case not handled by this code
There was a problem hiding this comment.
but that would be a bug in the code that uses this function, so a build-time issue, not a run-time issue. We can look at adding some build-time checks that all passed in responses are zero.
There was a problem hiding this comment.
i see, for the time being, we can probably edit the godoc of this function to say that Callers of this method should ensure that the resp parameter is zero/empty ?
There was a problem hiding this comment.
I added a isZeroProto check to the SendReceive function.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1958 +/- ##
==========================================
+ Coverage 55.45% 55.79% +0.34%
==========================================
Files 174 174
Lines 22297 22300 +3
==========================================
+ Hits 12365 12443 +78
+ Misses 8361 8277 -84
- Partials 1571 1580 +9
... and 8 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -223,10 +223,19 @@ func SendReceive(ctx context.Context, tcpNode host.Host, peerID peer.ID, | |||
| return errors.Wrap(err, "close write", z.Any("protocol", s.Protocol())) | |||
| } | |||
|
|
|||
| zeroResp := proto.Clone(resp) | |||
|
|
|||
| if err = reader.ReadMsg(resp); err != nil { | |||
There was a problem hiding this comment.
this will still return the errors that we are seeing in logs:
Peerinfo failed: read response: peer errored, no response
how is this allowing zero length responses?
There was a problem hiding this comment.
See legacyReadWriter ReadMsg
| return errors.Wrap(err, "read response") | ||
| } else if len(b) == 0 { | ||
| return errors.New("peer errored, no response") | ||
| return errors.Wrap(err, "read proto") | ||
| } |
There was a problem hiding this comment.
@dB2510 this was the zero length check that was erroring, it has been removed.
| // | ||
| // Note this function is inefficient for the negative case (i.e. when the message is not zero) | ||
| // as it copies the input argument. | ||
| func isZeroProto(m proto.Message) bool { |
corverroos
added
the
merge when ready
Fix regression introduced by length-delimited upgrades which assumed that legacy-stream-delimited protocols handled requests and responses the same while it didn't. It allowed zero length requests while it didn't allow zero-length responses. This aligns the new implementation with that.
category: bug
ticket: #1956