Several updates to refresh spdy-transport #61
Conversation
fix: remove safe-buffer * fix lint for latest standard * fix tests that broke in the dependency update * update deprecated assertions to the recommended methods
|
Hi @jacobheun, exceptional work, thank you so much for pushing this. Have you tested this with It seems that this upgrade requires to drop any Node.js version below 4. I am comfortable with that as those versions have left the LTS cycle for multiple years now. |
jacobheun
force-pushed
the
chore/update-deps
branch
from
6ebbdbf to
ebc5d02
Compare
| @@ -121,7 +121,13 @@ Scheduler.prototype.tickSync = function tickSync () { | |||
| debug('tick sync pending=%d', this.count, item.chunks) | |||
| for (var j = 0; j < item.chunks.length; j++) { | |||
| this.count-- | |||
| res = this.push(item.chunks[j]) | |||
| // TODO: handle stream backoff properly | |||
There was a problem hiding this comment.
What does properly mean in this case?
There was a problem hiding this comment.
Right now we could be iterating over n+1 chunks. If the nth chunk causes this.push to return false, we're still going to try and push another chunk. This will cause an error and violates back pressure design in the streams api. As soon as false is returned we need to respect that and wait for _read to get called again. I tried some other "quick fix" options, but it caused issues elsewhere. I need to spend more time getting up to speed on the transport to do a proper audit and refactor of the stream extensions.
|
@diasdavid ah right, I have not! I'll do that now and prep any PRs needed there. |
**What?** Point to the fixed version of spdy-transport from spdy-http2/spdy-transport#61. **Why?** Because ths breaks use of webpack-dev-server and we're aren't sure when the PRs will be merged.
**What?** Grab the spdy-transport fixes from spdy-http2/spdy-transport#61 **Why?** Because problems in spdy-transport have broken webpack-dev-server and there's no timeline for the fixes to be merged.
**What?** Grab the spdy-transport fixes from spdy-http2/spdy-transport#61 **Why?** Because problems in spdy-transport have broken webpack-dev-server and there's no timeline for the fixes to be merged.
|
@diasdavid I've got a PR open for node-spdy spdy-http2/node-spdy#351. It doesn't require any changes here and shows this working on the node LTS versions. There is an issue with node 11.1.0 due to internal stream changes, but I think that can be revisited as those changes stabilize. |
|
When we can expect for updated spdy package? |
|
@vytautas-pranskunas- spdy-http2/node-spdy#351 right now :) |
This PR does a few things (I can break the work out to separate PRs as needed) to start getting spdy-transport in a better spot for maintenance. There are still more things that are needed, but this should help us get started down that road.
standard_destroywill call abort when the inherited destroy is called.