net.js broken in node 8.8

[felixbrucker]

• Version: v8.8.0
• Platform: Linux staging 4.10.0-37-generic #41-Ubuntu SMP Fri Oct 6 20:20:37 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: net.js

I do not really have simple code, i noticed the problem while running storjshare-daemon, it happend about every 1-5 minutes:

net.js:401
  const prevWriteQueueSize = this._handle.writeQueueSize;
                                          ^

TypeError: Cannot read property 'writeQueueSize' of null
    at Socket._onTimeout (net.js:401:43)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:306:5)
    at Timer.listOnTimeout (timers.js:266:5)

This error seems to be introduced with the recent commit: a627c5f

[addaleax]

@apapirovski

[JohanvdWest]

Got the same. The code does not take into account the fact that the socket could be closed by another error (like the other side closing it).

Needs to check if _handle is valid.

[joyeecheung]

Refs: #15791

[apapirovski]

Bah, I thought I went through all the possibilities of _handle getting unset and couldn't find a way that it would in that code. Working on a PR.

[apapirovski]

Thanks for reporting @felixbrucker and @JohanvdWest. PR is at #16489

[depoulo]

Can anyone tell me whether I should roll back from v8.8.0 and wait for v8.8.1 do appear on Docker Hub?
Currently not seeing issues on our staging system with v8.8.0...

[Umkus]

@depoulo Same here. I see this in production on latest node:alpine, happens around every 15 minutes, sadly.

[apapirovski]

@depoulo @Umkus best to stay on 8.7.0 until 8.8.1 makes its way there. Sorry for the hassle.

[apapirovski]

@Umkus by the way if you happen to have more info re: what your code does and any dependencies, that would be helpful. I don't think we really have enough tests around this right now.

[Umkus]

Things are back to normal after pinning docker image to node:8.7-alpine.

@apapirovski
(Full disclosure: I'm not a professional nodejs developer)

Here's the (changed for readability) piece of code that I touched today, roughly before the app started to crash periodically:

let pings = {};

// asynchronously
if (!pings[hash]) {
    pings[hash] = setInterval(callbackFunc.bind(null, socket), 5000);
}

// asynchronously, after some time and under some conditions:
clearTimeout(pings[hash]);
delete pings[socket.room];

The only thing I actually changed was the addition of the delete statement right after clearing the timer. So that might be the culprit.

The app uses redis, socket.io and shuffle-array.

[apapirovski]

@Umkus It was definitely not your code. There was a full-blown, legitimate bug in 8.8.0 — I'm just trying to figure out the exact scenarios where it occurs. I'll have a look at socket.io and see if there's anything that can be stripped out into a node test to improve our coverage. Thanks for providing the info!