bindings-cpp icon indicating copy to clipboard operation
bindings-cpp copied to clipboard

Recursion+memory leak in unixRead()

Open dyarkovoy opened this issue 1 year ago • 2 comments

SerialPort Bindings Version

12.0.1

Node Version

v21.2.0

Electron Version

No response

Platform

Linux opi5b 5.10.110-rockchip-rk3588 #1.0.2 SMP Thu Apr 13 16:55:55 CST 2023 aarch64 GNU/Linux

Architecture

arm64

Hardware or chipset of serialport

16550A

What steps will reproduce the bug?

Simply opening /dev/ttyS9 on this hardware using the code below:

const debug=require('debug') debug.enable('*') const { SerialPort } = require('serialport') const port = new SerialPort({ path: '/dev/ttyS9', baudRate: 57600, })

port.on('data', function (data) { console.log('Data:', data) })

What happens?

initial unixRead() that is called on port open is trapped into recursive call to itself, because readAsync() always returns { bytesRead:0 }, see https://github.com/serialport/bindings-cpp/blob/39fd4be0904434a0ec72ee88e04d54bfa1645667/lib/unix-read.ts#L41C1-L44C1

The debug log confirms the said:

2023-11-26T14:55:04.624Z serialport/bindings-cpp loading LinuxBinding 2023-11-26T14:55:04.626Z serialport/stream opening path: /dev/ttyS9 2023-11-26T14:55:04.626Z serialport/bindings-cpp open 2023-11-26T14:55:04.628Z serialport/stream _read queueing _read for after open 2023-11-26T14:55:04.629Z serialport/bindings-cpp/poller Creating poller 2023-11-26T14:55:04.629Z serialport/stream opened path: /dev/ttyS9 2023-11-26T14:55:04.629Z serialport/stream _read reading { start: 0, toRead: 65536 } 2023-11-26T14:55:04.631Z serialport/bindings-cpp read 2023-11-26T14:55:04.631Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.632Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.632Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.632Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.632Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.633Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.633Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.633Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.633Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.633Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.634Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.634Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.634Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.634Z serialport/bindings-cpp/unixRead Starting read 2023-11-26T14:55:04.634Z serialport/bindings-cpp/unixRead Starting read

... and continues indefinitely

What should have happened?

unixRead() should return normally or throw

Additional information

I realize this is probably buggy hardware, but is it possible to work around this by implementing a simple recursion counter and throw if recursion deph exceeds 10 or so, for example?

dyarkovoy avatar Nov 26 '23 15:11 dyarkovoy

Faced the same issue. My solution was just to refactor unixRead() from recursion to simple while (true) {} and adding some delay in that loop. That solved both high cpu usage & "memory leak"

Kiliar avatar Jun 27 '24 12:06 Kiliar

The api contract is we have to return at least 1 byte of data. When your port opens with no data we have a tight loop with no waiting for a change in the file. I see how this behavior is a problem.

It makes sense if we get 0 bytes to wait for a readable notification from our poller like we do with retryable errors.

Since this is an async function this kind of recursion is safe to do and doesn't cause stack issues. The memory issue is obviously related but would need to profile it.

reconbot avatar Oct 18 '24 21:10 reconbot