Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bumped ws to v2.0.0. #106

Merged
merged 4 commits into from
Apr 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions echo-server.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

var http = require('http')
var websocket = require('./')
var server = null
Expand Down
6 changes: 3 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@

var Server = require('./server.js')
var server = require('./server.js')

module.exports = require('./stream.js')
module.exports.Server = Server
module.exports.createServer = Server
module.exports.Server = server.Server
module.exports.createServer = server.createServer
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
"duplexify": "^3.2.0",
"inherits": "^2.0.1",
"through2": "^2.0.0",
"ws": "^1.0.1",
"ws": "^2.2.3",
"xtend": "^4.0.0"
},
"devDependencies": {
Expand Down
33 changes: 33 additions & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,23 @@ Always convert to `Buffer` in Node.js before sending.

Default: `true`

##### `options.perMessageDeflate`

We recommend disabling the [per message deflate
extension](https://tools.ietf.org/html/rfc7692) to achieve the best
throughput.

Default: `true`

Example:

```js
var websocket = require('websocket-stream')
var ws = websocket('ws://realtimecats.com', {
perMessageDeflate: false
})
```

##### Other options

When used in node.js see the [ws.WebSocket documentation](https://github.com/websockets/ws/blob/master/doc/ws.md#class-wswebsocket)
Expand All @@ -57,6 +74,22 @@ function handle(stream) {
}
```

We recommend disabling the [per message deflate
extension](https://tools.ietf.org/html/rfc7692) to achieve the best
throughput:

```javascript
var websocket = require('websocket-stream')
var wss = websocket.createServer({
perMessageDeflate: false,
server: someHTTPServer
}, handle)

function handle(stream) {
fs.createReadStream('bigdata.json').pipe(stream)
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

It might makes sense to show how to disable it on the client (it's the same) as websocket-stream could be used only as a client.

## Run the tests

### Server-side tests
Expand Down
44 changes: 21 additions & 23 deletions server.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
'use strict'

var inherits = require('inherits')
var WebSocketServer = require('ws').Server
var stream = require('./stream')

module.exports = Server

function Server(opts, cb) {
if (!(this instanceof Server)) {
return new Server(opts, cb)
}

WebSocketServer.call(this, opts)

var proxied = false
this.on('newListener', function(event) {
if (!proxied && event === 'stream') {
proxied = true
this.on('connection', function(conn) {
this.emit('stream', stream(conn, opts))
})
class Server extends WebSocketServer{
constructor(opts, cb) {
super(opts)

var proxied = false
this.on('newListener', function(event) {
if (!proxied && event === 'stream') {
proxied = true
this.on('connection', function(conn) {
this.emit('stream', stream(conn, opts))
})
}
})

if (cb) {
this.on('stream', cb)
}
})

if (cb) {
this.on('stream', cb)
}
}

inherits(Server, WebSocketServer)

module.exports.Server = Server
module.exports.createServer = function(opts, cb) {
return new Server(opts, cb)
}
7 changes: 7 additions & 0 deletions stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ function WebSocketStream(target, protocols, options) {
var coerceToBuffer = options.binary || options.binary === undefined

function socketWriteNode(chunk, enc, next) {
// avoid errors, this never happens unless
// destroy() is called
if (socket.readyState !== WS.OPEN) {
Copy link
Contributor

@lpinca lpinca Apr 3, 2017

Choose a reason for hiding this comment

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

As I thought this has nothing to do ws@2, it is also needed for ws@1. The same issue also happens on master if you disable the permessage-deflate extension.

diff --git a/test.js b/test.js
index 18caa28..a0b10af 100644
--- a/test.js
+++ b/test.js
@@ -99,7 +99,9 @@ test('drain', function(t) {
   t.plan(1)
 
   echo.start(function() {
-    var client = websocket(echo.url, echo.options)
+    var client = websocket(echo.url, {
+      perMessageDeflate: false
+    })
 
     client.on('drain', function() {
       client.destroy()

In ws@2 this also happens with permessage-deflate enabled because data is not compressed unless the size is bigger than a configurable threshold (1024 bytes by default).

The problem with the "drain" test is that a single data chunk arrives on the server with many binary frames and a close frame. All frames are parsed on the same tick and when the close frame is processed, the readyState is set to CLOSING. The payloads of the processed binary frames are pushed to the proxy, but when socketWriteNode() is called the readyState is already CLOSING.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the explanation.

next()
return
}

if (coerceToBuffer && typeof chunk === 'string') {
chunk = new Buffer(chunk, 'utf8')
}
Expand Down