Fix a bug when parsing multiple responses #115
Conversation
|
Thanks for the PR @bwdeng, have you faced this problem while using the library? We have been using KafkaJS for a while and I haven't seen this behavior. |
src/network/connection.js
Outdated
| // The full payload is loaded, erase the temporary buffer | ||
| this.buffer = Buffer.alloc(0) | ||
| if (this.authHandlers) { | ||
| return this.authHandlers.onSuccess(data.slice(0, this.offset + byteLength)) |
There was a problem hiding this comment.
byteLength isn't defined, this is breaking the tests
There was a problem hiding this comment.
good catch! copy-paste bug...
| const correlationId = decoder.readInt32() | ||
| const payload = decoder.readAll() | ||
| const entry = this.pendingQueue[correlationId] | ||
| delete this.pendingQueue[correlationId] |
There was a problem hiding this comment.
I wonder if we should remove the previous data from the buffer, I fear to end up leaking memory.
There was a problem hiding this comment.
I think we are okey, since the buffer is reset using decoder.readAll() which only copies (not slicing) the rest of the data. So the original buffer could be collected by GC once the response gets consumed.
|
@tulios It's more likely to happen if multiple requests were sent currently. We were facing a problem when starting a new consumer group. Sometimes, it causes the response of |
|
@bwdeng the code makes sense, I just wanted to confirm. I want to rename some things (like Thanks again! |
|
@bwdeng Thanks again, I'm releasing o/ |
The previous implementation was assuming that the
rawDataonly contains the bytes of a single response. This could cause response loss when mutiple reponses are concatenated into onerawData.