[fix] Undeterministic error in polling buffer processing

[raulmt]

The kind of change this PR does introduce

• a bug fix
• a new feature
• an update to the documentation
• a code change that improves performance
• other

Current behaviour

Sometimes chunks stops being a Buffer when onData expects it to be one

New behaviour

Ensure all assignments for chunks are Buffer when they should, even when onData is supposed to not be called again.

Other information (e.g. related issues)

#528 for details

[darrachequesne]

Hi! Couldn't we remove that line instead? req.connection.destroy(); should trigger an onClose event, right? I mean:

  function cleanup () {
-   chunks = isBinary ? new Buffer(0) : '';
    req.removeListener('data', onData);
    req.removeListener('end', onEnd);
    req.removeListener('close', onClose);
-   self.dataReq = self.dataRes = null;
+   self.dataReq = self.dataRes = chunks = null;
  }

  function onClose () {
    cleanup();
    self.onError('data request connection closed prematurely');
  }

  function onData (data) {
    var contentLength;
    if (typeof data === 'string') {
      chunks += data;
      contentLength = Buffer.byteLength(chunks);
    } else {
      chunks = Buffer.concat([chunks, data]);
      contentLength = chunks.length;
    }

    if (contentLength > self.maxHttpBufferSize) {
-     chunks = isBinary ? new Buffer(0) : '';
      req.connection.destroy();
    }
  }

[raulmt]

Hi @darrachequesne . At first I thought "no, because if onClose is called and sets chunks to null, then the next call to onData if any will fail with the same problem (chunks not being a Buffer). But as I described in #528 the only way I can think of this could happen is if onData callback is already in the callback queue before consequences related to destroying the socket even happen. So if that's the case, the onClose callback because of destroying the socket should be warranted to happen after that eventual onData next call, so chunks would still be a Buffer…

I tested what you proposed and it seems to work too, both in cases where max buffer is reached and when it's not. I updated this PR with your suggestion since it's simpler. Thanks :D

[raulmt]

@darrachequesne actually I had to add that line setting an empty buffer when self.maxHttpBufferSize is reached back… onEnd is still called, and before onClose, so self.onData is called, and that makes a couple of tests fail because we shouldn't emit data if the max http buffer size was reached. I kept the change inside cleanup though…

[darrachequesne]

Well that makes sense, thanks!