Fixes infinite loop triggered by oversized metrics #1
Conversation
Adds a length check to `send` that checks whether the metric being sent is longer than the max buffer size. An exception is raised to the caller (*not* in the reaper thread) if the metric is too large. In order to check the length before adding the item to the reaper thread, the UTF8 building had to move to be done in the calling thread rather than the background. Prior to this, if a chunk larger than the max buffer size made it into `builderAction` it would cause an infinite loop. It would try to flush existing chunks to make room for the new one, but would then get stuck because the oversize chunk was still too big. The exact same thing would happen on the next `builderAction` iteration (and so forth). Fixes iand675#32.
src/Network/StatsD/Datadog.hs
Outdated
| then (finalizeChunks chunks, s) | ||
| else go newSize (bs : chunks) rest | ||
| (bs Seq.:< rest) -> | ||
| let newChuckSize = B.length bs |
src/Network/StatsD/Datadog.hs
Outdated
| (bs Seq.:< rest) -> | ||
| let newChuckSize = B.length bs | ||
| newTotalSize = newChuckSize + accum | ||
| in if newChuckSize > maxBufSize |
There was a problem hiding this comment.
Doesn't this need to be newChuckSize + 1 > maxBufSize to account for the newline that is added on line 452 in the send function?
There was a problem hiding this comment.
No, because the newline is already part of the chunk at this point.
| @@ -52,3 +55,39 @@ spec = describe "StatsD spec" $ do | |||
| case val of | |||
| Just _ -> pure () | |||
| Nothing -> expectationFailure "Did not receive DogStatsD event" | |||
| it "does not go into an infinite loop when trying sending a metric larger than a UPD packet" $ do | |||
| let longText = Text.replicate 65507 "x" | |||
There was a problem hiding this comment.
I think it would be better to get the 65507 value from the settings when you create it on line 61 with dogStatsSettingsBufferSize, in case this hard-coded value is changed in the future.
There was a problem hiding this comment.
I was a bit torn on that since the value is really related to the max size of a UDP packet, which isn't going to change. I'll think through it and revise it somehow before I post the PR upstream.
There was a problem hiding this comment.
That's a good point. Something else you might want to consider is this line from the datadog documentation:
In some scenarios, however, you may wish to send smaller packets.
Although, since you are using the default configuration, that might not be a factor for this unit test.
There was a problem hiding this comment.
Ultimately I decided to do nothing here, because the matching the actual rendered chunk length in statsd format to the configured buffered size is delicate, and could easily be broken in the future by changes to renderEvent.
Leaving the value at the maximum possible UDP packet data size guarantees that this test will produce a "too large" packet no matter what (realistic) default value is configured in the library in the future.
|
I'm going to close this, squash the commits on a new branch, and then submit the PR upstream. |
Adds a length check to
sendthat checks whether the metric being sentis longer than the max buffer size. An exception is raised to the caller
(not in the reaper thread) if the metric is too large. In order to
check the length before adding the item to the reaper thread, the UTF8
building had to move to be done in the calling thread rather than the
background.
Prior to this, if a chunk larger than the max buffer size made it into
builderActionit would cause an infinite loop. It would try to flushexisting chunks to make room for the new one, but would then get stuck
because the oversize chunk was still too big. The exact same thing
would happen on the next
builderActioniteration (and so forth).Fixes iand675#32.