Skip to content

Commit

Permalink
fix: support onStreamRead for node 11.1 and above
Browse files Browse the repository at this point in the history
  • Loading branch information
@Cellule
Cellule committed Dec 10, 2019
1 parent 219d451 commit 4e4aae8
Showing 1 changed file with 23 additions and 3 deletions.
26 changes: 23 additions & 3 deletions lib/handle.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,19 @@ var Buffer = require('buffer').Buffer
var Queue = require('./queue')

// Node.js version
var match = /^v(\d+)\.(\d+)\./.exec(process.version)
var version = match ? Number(match[1]) + Number('0.' + match[2]) : 11
var onreadMode = version >= 11.1 ? 2 : 1
var mode = 'modern'

var setNRead
if (onreadMode === 2) {
var sw = process.binding('stream_wrap')
setNRead = function (nread) {
sw.streamBaseState[sw.kReadBytesOrError] = nread
}
}

function Handle (stream, options) {
EventEmitter.call(this)

Expand All @@ -31,6 +42,15 @@ Handle.create = function create (stream, options) {
return new Handle(stream, options)
}

Handle.prototype._onread = function _onread (nread, buffer) {
if (onreadMode === 1) {
this.onread(nread, buffer)
} else {
setNRead(nread)
this.onread(buffer)
}
}

Handle.prototype._queueReq = function _queueReq (type, req) {
return this.pending.append(type, req)
}
Expand Down Expand Up @@ -81,17 +101,17 @@ var uv = process.binding('uv')
Handle.prototype._flow = function flow () {
var self = this
this._stream.on('data', function (chunk) {
self.onread(chunk.length, chunk)
self._onread(chunk.length, chunk)
})

this._stream.on('end', function () {
self.onread(uv.UV_EOF, Buffer.alloc(0))
self._onread(uv.UV_EOF, Buffer.alloc(0))
})

this._stream.on('close', function () {
setImmediate(function () {
if (self._reading) {
self.onread(uv.UV_ECONNRESET, Buffer.alloc(0))
self._onread(uv.UV_ECONNRESET, Buffer.alloc(0))
}
})
})
Expand Down

7 comments on commit 4e4aae8

@waffledonkey
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I confirmed that these changes (transposed manually) fix this issue:

internal/buffer.js:940
class FastBuffer extends Uint8Array {}
^

RangeError: Invalid typed array length: -4095
    at new Uint8Array (<anonymous>)
    at new FastBuffer (internal/buffer.js:940:1)
    at Handle.onStreamRead [as onread] (internal/stream_base_commons.js:181:19)
    at Stream.<anonymous> (REDACTED/node_modules/handle-thing/lib/handle.js:115:10)
    at Stream.emit (events.js:215:7)
    at endReadableNT (REDACTED/node_modules/spdy-transport/node_modules/readable-stream/lib/_stream_readable.js:1077:12)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)

references: #363 and #350, others?

Unrelated to the error, but related to recent changes response.js line 18 is now throwing a deprecated message and needs to be updated (eventually) to this.getHeaders(), but since you don't care about the result you can just test the existence of the method -- versus invoking it.

Thank you for your effort and Happy Holidays!

@aprilmintacpineda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this fix been released? I'm just ran into this error.

@LucaNerlich
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no it has not.

@Quirksmode
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will this be released? I also have the same issue and it means I am unable to push my app live until I can use the latest version of Node (host only supports Node version >= 12)

@nemphys
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried this manually and it indeed fixes compatibility of spdy with Node >= 11 (tried with the latest 12.x).
Please publish this asap, many people will thank you :-)

@cesarchefinho
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please commit this update. Many people are waiting for use spdy in node 12lts.

There are no justification to a bug like this with a year age and the correction not comitted.

@mindaugasvcs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pure joy of using node :D

Please sign in to comment.