Prevent busy-loop when message are larger than available socket-buffer #92
Conversation
|
Thank you for looking into this 🙌 Great idea using the I'm wondering if we could simplify this class even more. Instead of manually managing a separate buffer, what if we leveraged the internal buffer of the Socket? We could simply check private _onReadable() {
while (!this._readingPaused) {
if (this._incomingBodyLength === null) {
if (HEADER_LENGTH > this._socket.readableLength) return; // Wait for more data
// Read message length header
const lenBuf = this._socket.read(HEADER_LENGTH);
this._incomingBodyLength = lenBuf.readInt32LE() - HEADER_LENGTH;
}
// If expected body is longer than readable buffer
if (this._incomingBodyLength > this._socket.readableLength) return; // Wait for more data
// Read message body
const body = this._socket.read(this._incomingBodyLength);
this._pushCompleteMessage(body);
this._incomingBodyLength = null;
}
} |
|
That is a much cleaner solution, I was under the impression that the buffer wasn't appended until it was cleared. Which seam to be wrong. I tried your solution, but ran in to some issues.. Your code (slightly modified): _incomingBodyLength: number|null = null;
_onReadable() {
while (!this._readingPaused) {
if (this._incomingBodyLength === null) {
if (HEADER_LENGTH > this._socket.readableLength) return; // Wait for more data
// Read message length header
const lenBuf = this._socket.read(HEADER_LENGTH);
this._incomingBodyLength = lenBuf.readInt32LE() - HEADER_LENGTH;
}
// If expected body is longer than readable buffer
console.log(this._incomingBodyLength, this._socket.readableLength);
if (this._incomingBodyLength > this._socket.readableLength) return; // Wait for more data
console.log("-- got a message --")
// Read message body
const body = this._socket.read(this._incomingBodyLength);
this._pushMessage(body);
this._incomingBodyLength = null;
}
}gives this output: npx ts-node samples/typescript/facilities-waypoints.ts
304 304
-- got a message --
Connected to sim!
41057 14476
41057 40540Then the script exists with exit-code 0, no error, no output, no noting.. But ONCE out of ~10, it worked (and script stayed alive).. So I have no idea what's happening now.. (And my lunch is almost over for today) I'll look into it more when I get a chance, but this is probably the correct path to go. |
|
Hmm, it seems the 'readable' event might stop when the socket's buffer reaches a certain length/“watermark” (Node.js Stream Documentation). Perhaps that it what happens. In that case it might be as simple as raising the watermark so it can buffer any SimConnect message size - the only concern is I don’t know if there’s any documented limit to how big a SimConnect message body can be 🤔 |
|
That looked really promising, finally got a few minutes tonight to try it out, no dice though.. First tried reading it: Odd thing though, tonight the "batches" tended to be smaller, ie. the _onReadable was generally (but not always) called more times (regardless of waterMark), like this: It also tended to work correctly more than last time I tried this, maybe 4/10.. (Sidenote, the version currently in PR has worked without incident for several hours, even with the full list of 40 000 airports) I'll look into it more when I get a chance! ... Couldn't let it go quite yet,, had a faint memory of NODE_DEBUG.. Not often I need to go that deep.. I'll look into it more when I get a chance! (for real this time) |
|
Okay, I finally tried connecting to the sim from an external laptop, and I got the same result as you. I'm leaning back towards your idea of keeping a local copy of the received data after all. It might be a safer solution than relying on the internal magic happening in the Socket class 😅 I tried to simplify it a bit - does this code work for you? _onReadable() {
while (!this._readingPaused) {
const chunk: Buffer | null = this._socket.read();
if (chunk === null) break;
this._dataBuffer = Buffer.concat([this._dataBuffer, chunk]);
while (this._dataBuffer.length >= HEADER_LENGTH) {
const totalMessageSize: number = this._dataBuffer.readInt32LE(0);
if (this._dataBuffer.length >= totalMessageSize) {
const messageBody: Buffer = this._dataBuffer.slice(
HEADER_LENGTH,
totalMessageSize
);
const simConnectMessage: SimConnectMessage = {
protocolVersion: messageBody.readInt32LE(0),
packetTypeId: messageBody.readInt32LE(4),
data: new RawBuffer(messageBody.slice(8)),
};
const pushOk = this.push(simConnectMessage);
if (!pushOk) {
this._readingPaused = true;
break; // Pause reading if consumer is slow
}
this._dataBuffer = this._dataBuffer.slice(totalMessageSize); // Remove processed message from the buffer
} else {
break; // Not enough data for a complete SimConnect message, break out of the loop
}
}
}
} |
|
Finally got some time to test this! Yes that seem to be working nicely! Haven't had a chance to do any testflights but my application works nicely with it. I suggest you create a new branch/PR with that, then we'll close this one. |
|
@AndreKlang , thanks for your feedback! I have created #96 which uses that last snippet. |
Fixes #91
Please consider this a proof of concept, I've confirmed that this fixes my issue as described in #91. The example there now works, and the
facilities.tssample also work as expected. I've done a short 15min test-flight, with no issue. (That's unfortunately the extent of my testing so far)The
_onReadablemethod is a bit long and complicated now in my opinion, so I'd refactor that, but I don't have the time right now. And maybe you'd like to do that yourself any way. So I figured I'd keep this PR simple and to the point.