Repository navigation
Conversation
ibc
left a comment
ibc
left a comment
There was a problem hiding this comment.
As commented above I'd prefer if we clean the builder upon failure rather than on next usage.
Also, some kinda related issue:
In stats it could now happen that some "bitrate" field is super big so it becomes automatically a BigInt during the FBS to JS conversion (automatically). The problem is that the application usually uses JSON.stringify(stats) and such a function will fail if any of the json fields is a BigInt.
It's unlikely that we can generate stats with bitrate fields bigger than the biggest normal number supported in Node/JS, because it would literally be 9 Tera bytes per second which is crazy. But perhaps we should always invoke Number(fbsBitrateValue) when converting from FBS to JS just in case.
| * serialization (which would otherwise break every future message). | ||
| */ | ||
| get bufferBuilder(): flatbuffers.Builder { | ||
| this.#bufferBuilder.clear(); |
There was a problem hiding this comment.
I don't love this solution because it basically means that, in case of corrupted buffer, we leave it that way until the next time we need to use the buffer.
| ); | ||
| } | ||
|
|
||
| // If there is no body then the buffer builder holds nothing we need, so |
There was a problem hiding this comment.
This is also post-defensive approach that we are supposed to hate.
| // maxBitrate is optional, it can be a number or a bigint. | ||
| const maxBitrate: number | bigint | undefined = encoding.maxBitrate; | ||
|
|
||
| if (!maxBitrate) { |
There was a problem hiding this comment.
This is not valid since 0 is a valid value but it enters this condition. It works by accident but this solution is not valid for other similar cases such as transport.setMaxIncomingBitrate(bitrate) where 0 means "no limit".
|
|
||
| // Add max bitrate. | ||
| if (encoding.maxBitrate !== undefined) { | ||
| if (encoding.maxBitrate) { |
There was a problem hiding this comment.
Again this condition breaks the valid case of value 0.
|
Also, same problem may happen in all methods such as transport.setMax/MinIncoming/OutgoingBitrate(). And there the conditions/checks written in this PR for encoding.maxBitrate are invalid as said above. I would ask to improve them here as well because 0 is a valid value and it's not "lack of value". |
This would mean filling the codebase with try/catches everywhere just to clean the buffer builder. The error came because the the value was not validated by us correctly. It was a bug on our end. And by cleaning up the buffer before, and not later (we just moved the call to |
Ok I but that. But my other concerns are real and valid IMHO. |
|
Some of the tests in |
2 participants
Basically clean the buffer builder at the beginning of message creation rather than at the end. In order to cleanup the wrong state left by a previous message construction.