fix: correct buffers concatenation in the socket manager (Deluge)

[shamelin]

Description

There currently is an issue with the concatenation of buffers in the clientRequestManager.ts of the Deluge service, more specifically at L129. If a client has a lot of torrents (in the current case, 588 torrents crash the software - though this is not the minimal amount to reproduce the issue), Flood will crash at boot up when it receives the packets from Deluge. Below is an explanation of the issue more specifically:

I have debugged the line to see what goes wrong. If you take a look at the first screenshot, you can see that rpcBuffer.length = 16379, chunk.length = 16384 and rpcBufferSize = 50328. rpcBuffer is the content of the buffer currently in memory (captured from a previous packet received), chunk is the new buffer received by Flood, and rpcBufferSize is the expected total size of the buffer.

Based on those values, we should expect the new size of rpcBuffer to be 16379 + 16384 = 32763. There's other packets to obtain before being able to process the whole buffer at L140, which checks that the current length of rpcBuffer is equal of greater than rpcBufferSize.

The issue resides in the call made to Buffer.concat, specifically the second parameter. Taking a look at the second screenshot, you can see that the size of rpcBuffer is now 50328, when it should've been 32763.

Taking a look at the documentation for the function Buffer.concat(list[, totalLength]), it mentions: totalLength <integer>: Total length of the Buffer instances in list when concatenated.. We can observe that behavior by taking a look at the end of the buffer chunk and rpcBuffer after Buffer.concat is called (50 last elements for both):

END OF chunk (last 50 entries):
{
  "type": "Buffer",
  "data": [
    193, 131, 55, 30, 78, 175, 113, 225, 171, 54, 136, 230, 240, 202, 216, 137, 
    105, 235, 20, 114, 112, 184, 161, 42, 123, 99, 150, 101, 33, 146, 15, 53, 226, 
    210, 199, 164, 143, 7, 150, 166, 43, 77, 87, 10, 248, 151, 102, 230, 42, 181
  ]
}

END OF rpcbuffer (last 50 entries):
{
  "type": "Buffer",
  "data": [
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 
    0, 0, 0, 0, 0, 0, 0, 0
  ]
}

This means that by adding rpcBufferSize as the total length of the buffer when calling Buffer.concat(list[, totalLength]), it will add 0s at the end of the new buffer until its length is the expected value. This is not the expected behavior here, as we will receive subsequent packets after the current one that we will have to append to the buffer. Hence, I have modified the code to specify rpcBufferSize for the total length only if rpcBuffer.length + chunk.length is bigger (or equal to) than rpcBufferSize. We could also completely remove the second parameter, but it wouldn't be ideal.

Related Issue

See #788 for a tracking of the issue.

Screenshots

FIRST SCREENSHOT

SECOND SCREENSHOT

Types of changes

This is a bug fix and should not break the typical behavior of the project. It should allow Flood to handle a more subsequent load of torrents at the same time.

• Breaking change (changes that break backward compatibility of public API or CLI - semver MAJOR)
• New feature (non-breaking change which adds functionality - semver MINOR)
• Bug fix (non-breaking change which fixes an issue - semver PATCH)

[trim21]

maybe we should not use a self baked rpc call but use axios instead

[trim21]

will this fix 788？

[shamelin]

Yup! I tested it locally and it worked. I can also take a look at replacing this altogether with axios as you suggested in a separate issue :D