Add method to RelayFrame to reveal Arg2 offset #746
Conversation
Add a method "Arg2EndOffset" to RelayFrame interface so that we could know how much space Arg2 is used. This is the first step to know if it's possible to add more methods in Relay code to allow Relayer to inject more headers or read the headers to decide which peer to forward to. NOTE: this is a breaking change to RelayFrame interface. Related to uber#745. Will use this change to know Arg2 usage.
|
|
Codecov Report
@@ Coverage Diff @@
## dev #746 +/- ##
==========================================
+ Coverage 88.14% 88.39% +0.24%
==========================================
Files 40 40
Lines 4058 4066 +8
==========================================
+ Hits 3577 3594 +17
+ Misses 366 360 -6
+ Partials 115 112 -3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Add test cases for:
- 0 length arg2
- positive length arg2 (with and without overflow to another frame)
- max length arg2 (with and without overflow to another frame)
We might also want to add arg2 starting offset so we can use [end-start] as an estimate of the header size for metrics (related to T2938739)
Not quite sure I understand the difference between positive length and max length, but I've added tests for 0 length arg2 and arg2 which spans across frames. |
relay_messages.go
Outdated
| cr.arg2StartOffset = cur + 2 | ||
| cr.arg2EndOffset = cur + 2 /*arg2 len*/ + int(binary.BigEndian.Uint16(f.Payload[cur:cur+2])) | ||
| // arg2 is fragmented if we don't see arg3 in this frame. | ||
| cr.isArg2Fragmented = int(cr.Header.FrameSize()) <= (FrameHeaderSize+cr.arg2EndOffset) && f.Payload[_flagsIndex]&hasMoreFragmentsFlag != 0 |
There was a problem hiding this comment.
instead of using frame size here, if you use PayloadSize(), then you won't need to add FrameHeaderSize and it'll be a litlte simpler.
similarly, don't access the flags directly, use the method HasMoreFragments()
cr.isArg2Fragmented = int(cr.Header.PayloadSize()) <= cr.arg2EndOffset && cr.HasMoreFragments()can we add a test for case where a call doesn't set the has more fragments flag, and doesn't have arg3
There was a problem hiding this comment.
Good point - I think I made the mistake that the offset is from start of the payload to Arg2, not from the start of frame (which includes header).
Added test for the flag is not set and does not have arg3.
relay_messages.go
Outdated
| @@ -183,6 +193,18 @@ func (f lazyCallReq) HasMoreFragments() bool { | |||
| return f.Payload[_flagsIndex]&hasMoreFragmentsFlag != 0 | |||
| } | |||
|
|
|||
| // Arg2EndOffset returns the offset from start of payload to the end of Arg2 | |||
| // in bytes, and whether there are more data from other frames. | |||
There was a problem hiding this comment.
"more data from other frames" --> let's make this more clear. we only return true here if there's more frames and we haven't yet started arg3, right?
|
As discussed offline, let's follow this up with integration tests that use the Separately, can you file an issue for ensuring we don't ever "overflow" reads? If we send a corrupt packet, it may cause out-of-bounds access. |
Created #747 |
Add a method "Arg2EndOffset" to RelayFrame interface so that we could
know how much space Arg2 is used. This is the first step to know if it's
possible to add more methods in Relay code to allow Relayer to inject
more headers or read the headers to decide which peer to forward to.
NOTE: this is a breaking change to RelayFrame interface.
Related to #745. Will use this change to know Arg2 usage.