openbci-archive/OpenBCI_NodeJS

State not cleaned up on socket close

Closed this issue · 0 comments

https://github.com/baffo32/OpenBCI_NodeJS/tree/1.4.x-socket-close-resources

  • A test should be added to ensure write functions fail and connection state is disconnected, after connection close.
  • A test should be added for close before writes are complete, that the data is not written on the next connection
  • .connected = false is only set when disconnect() is called, so invalid state will be entered if the dongle is yanked etc. This should be set in the close listener, and in the error listener, or in a unified internal function such as a ._disconnected().
  • investigate use and behavior of the function _reset() and see how it relates to this. It may be redundant. If not, give it a more descriptive name, and call it if needed in the disconnection handler.
  • Some code checks for connection with this.serial, and some with this.connected. Because the serial port library does not allow to re-open a serial port to a different path, it is somewhat appropriate that presence of the serial object be roughly equated with connection (path may change if dongle is yanked and reinserted). Still, this code should be unified to have one canonical way of detecting connection, for maintainability: perhaps replace this.connected with a public API function that checks for this.serial. At the moment the this.serial code is incorrect because of the next task item.
    • An issue here is that the serial port has more states that simply here or not here. Notably it can be present and not connected immediately after constructor call. We can code such that it is never present and 'disconnecting', but this could be hard in the future for others to continue to maintain.
    • So it is reasonable for connection state to be separate from serial presence. But of note is that when connected, the serial will always be present. The reverse is not true. So really, if isConnected() checks both, I just need to check the uses of it that they are doing what is intended, given these state relationships. I haven't come up with a possible situation where that wouldn't be the case.
    • Crucially, this.serial must not be used unless this.isConnected() is checked first. Perhaps this is obvious, though.
  • this.serial is not removed, nor are its error, data, and close listeners, upon disconnection. This should be done in parallel with, or in replacement of, this.connected = false.
  • the write function state should be cleaned up when socket is closed. the command queue should be emptied, and any other cleanup needed. If #91 is implemented first, reject write promises when socket is closed.
  • A test should be added for behavior choice on close when writes are pending: that they either are, or are not, written out on close.
  • ensure no event listeners are still active when serial object is lost
  • ensure no running timeouts, intervals, etc are still active after disconnection