Skip to content

Commit

Permalink
Add threshold option to compression.
Browse files Browse the repository at this point in the history
  • Loading branch information
Tom Atkinson committed Oct 24, 2016
1 parent 63314f7 commit 6b3904b
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 28 deletions.
1 change: 1 addition & 0 deletions doc/ws.md
View file
Expand Up @@ -63,6 +63,7 @@ If `handleProtocols` is not set then the handshake is accepted regardless the va
* `serverMaxWindowBits` Number: The value of windowBits.
* `clientMaxWindowBits` Number: The value of max windowBits to be requested to clients.
* `memLevel` Number: The value of memLevel.
* `threshold` Number: Payloads smaller than this will not be compressed. Default 1024 bytes.

If a property is empty then either an offered configuration or a default value is used.

Expand Down
1 change: 1 addition & 0 deletions lib/PerMessageDeflate.js
View file
Expand Up @@ -18,6 +18,7 @@ class PerMessageDeflate {
this._deflate = null;
this.params = null;
this._maxPayload = maxPayload || 0;
this.threshold = this._options.threshold === undefined ? 1024 : this._options.threshold;
}

/**
Expand Down
43 changes: 19 additions & 24 deletions lib/Sender.js
View file
Expand Up @@ -78,7 +78,7 @@ class Sender {
*/
doPing (data, options) {
var mask = options && options.mask;
this.frameAndSend(0x9, data || '', true, mask);
this.frameAndSend(0x9, data ? Buffer.from(data.toString()) : null, true, mask);
if (this.extensions[PerMessageDeflate.extensionName]) {
this.messageHandlerCallback();
}
Expand All @@ -104,7 +104,7 @@ class Sender {
*/
doPong (data, options) {
var mask = options && options.mask;
this.frameAndSend(0xa, data || '', true, mask);
this.frameAndSend(0xa, data ? Buffer.from(data.toString()) : null, true, mask);

This comment has been minimized.

Sign in to view

Copy link
@lpinca

lpinca Nov 2, 2016 •
edited

Member

@Nibbler999 I spotted an issue with this while running the Autobahn test suite.
If data is a buffer, a new buffer is created after converting it to a utf8 string changing the original buffer contents.

> var b1 = crypto.randomBytes(4)
undefined
> var b2 = Buffer.from(b1.toString())
undefined
> b1
<Buffer b4 a5 52 0b>
> b2
<Buffer ef bf bd ef bf bd 52 0b>

I think a clean way to fix this would be to extract the data handling if at line 132 into a standalone function which can be reused in doPing, doPong and send.

What do you think?

This comment has been minimized.

Sign in to view

Copy link
@lpinca

lpinca Nov 2, 2016

Member

#881.
if (this.extensions[PerMessageDeflate.extensionName]) {
this.messageHandlerCallback();
}
Expand All @@ -129,6 +129,17 @@ class Sender {
}
if (finalFragment) this.firstFragment = true;

if (data && !Buffer.isBuffer(data)) {
if ((data.buffer || data) instanceof ArrayBuffer) {
data = getBufferFromNative(data);
} else {
if (typeof data === 'number') {
data = data.toString();
}
data = Buffer.from(data);
}
}

if (this.extensions[PerMessageDeflate.extensionName]) {
this.enqueue([this.sendCompressed, [opcode, data, finalFragment, mask, compress, cb]]);
} else {
Expand All @@ -142,6 +153,11 @@ class Sender {
* @api private
*/
sendCompressed (opcode, data, finalFragment, mask, compress, cb) {
if (data && data.length < this.extensions[PerMessageDeflate.extensionName].threshold) {
this.frameAndSend(opcode, data, finalFragment, mask, false, cb);
this.messageHandlerCallback();
return;
}
this.applyExtensions(data, finalFragment, this.compress, (err, data) => {
if (err) {
if (cb) cb(err);
Expand All @@ -159,32 +175,13 @@ class Sender {
* @api private
*/
frameAndSend (opcode, data, finalFragment, maskData, compressed, cb) {
var canModifyData = false;

if (!data) {
var buff = [opcode | (finalFragment ? 0x80 : 0), 0 | (maskData ? 0x80 : 0)]
.concat(maskData ? [0, 0, 0, 0] : []);
sendFramedData(this, new Buffer(buff), null, cb);
return;
}

if (!Buffer.isBuffer(data)) {
if ((data.buffer || data) instanceof ArrayBuffer) {
data = getBufferFromNative(data);
} else {
canModifyData = true;
//
// If people want to send a number, this would allocate the number in
// bytes as memory size instead of storing the number as buffer value. So
// we need to transform it to string in order to prevent possible
// vulnerabilities / memory attacks.
//
if (typeof data === 'number') data = data.toString();

data = new Buffer(data);
}
}

var dataLength = data.length;
var dataOffset = maskData ? 6 : 2;
var secondByte = dataLength;
Expand All @@ -197,6 +194,7 @@ class Sender {
secondByte = 126;
}

var canModifyData = opcode === 1 || compressed;
var mergeBuffers = dataLength < 32768 || (maskData && !canModifyData);
var totalLength = mergeBuffers ? dataLength + dataOffset : dataOffset;
var outputBuffer = new Buffer(totalLength);
Expand Down Expand Up @@ -276,9 +274,6 @@ class Sender {
*/
applyExtensions (data, fin, compress, callback) {
if (compress && data) {
if ((data.buffer || data) instanceof ArrayBuffer) {
data = getBufferFromNative(data);
}
this.extensions[PerMessageDeflate.extensionName].compress(data, fin, callback);
} else {
callback(null, data);
Expand Down
21 changes: 18 additions & 3 deletions test/Sender.test.js
View file
Expand Up @@ -30,7 +30,7 @@ describe('Sender', function() {
it('does not modify a masked text buffer', function() {
var sender = new Sender({ write: function() {} });
var text = 'hi there';
sender.frameAndSend(1, text, true, true);
sender.frameAndSend(1, Buffer.from(text), true, true);
text.should.eql('hi there');
});

Expand All @@ -41,13 +41,13 @@ describe('Sender', function() {
done();
}
});
sender.frameAndSend(1, 'hi', true, false, true);
sender.frameAndSend(1, Buffer.from('hi'), true, false, true);
});
});

describe('#send', function() {
it('compresses data if compress option is enabled', function(done) {
var perMessageDeflate = new PerMessageDeflate();
var perMessageDeflate = new PerMessageDeflate({ threshold: 0 });
perMessageDeflate.accept([{}]);

var sender = new Sender({
Expand All @@ -61,6 +61,21 @@ describe('Sender', function() {
sender.send('hi', { compress: true });
});

it('does not compress data for small payloads', function(done) {
var perMessageDeflate = new PerMessageDeflate();
perMessageDeflate.accept([{}]);

var sender = new Sender({
write: function(data) {
(data[0] & 0x40).should.not.equal(0x40);
done();
}
}, {
'permessage-deflate': perMessageDeflate
});
sender.send('hi', { compress: true });
});

it('Should be able to handle many send calls while processing without crashing on flush', function(done) {
var messageCount = 0;
var maxMessages = 5000;
Expand Down
2 changes: 1 addition & 1 deletion test/WebSocket.test.js
View file
Expand Up @@ -2226,7 +2226,7 @@ describe('WebSocket', function() {
describe('#terminate', function() {
it('will raise error callback, if any, if called during send data', function(done) {
var wss = new WebSocketServer({port: ++port, perMessageDeflate: true}, function() {
var ws = new WebSocket('ws://localhost:' + port, {perMessageDeflate: true});
var ws = new WebSocket('ws://localhost:' + port, {perMessageDeflate: { threshold: 0 }});
var errorGiven = false;
ws.on('open', function() {
ws.send('hi', function(error) {
Expand Down

8 comments on commit 6b3904b

@lpinca
Copy link
Member

@lpinca lpinca commented on 6b3904b Oct 25, 2016 •
edited

Choose a reason for hiding this comment

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

I'm not sure why but this commit introduced an utf-8 validation error when running bench/speed.js. We need to investigate why this is happening.

@Nibbler999
Copy link
Member

Choose a reason for hiding this comment

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

I can't reproduce that.

@lpinca
Copy link
Member

@lpinca lpinca commented on 6b3904b Oct 25, 2016

Choose a reason for hiding this comment

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

Weird, I can reproduce it with 100% accuracy on macOS by only enabling this config.

@lpinca
Copy link
Member

@lpinca lpinca commented on 6b3904b Oct 25, 2016 •
edited

Choose a reason for hiding this comment

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

It seems that the issue happens because data is changed in place, so the next time send is called, the mask is applied on an already masked buffer.

@Nibbler999
Copy link
Member

Choose a reason for hiding this comment

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

I see, canModifyData is incorrect.

@Nibbler999
Copy link
Member

Choose a reason for hiding this comment

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

Added a fix for this, maybe revisit it later.

@lpinca
Copy link
Member

@lpinca lpinca commented on 6b3904b Nov 4, 2016

Choose a reason for hiding this comment

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

@Nibbler999 there might be another issue with this.

Imagine a fragmented message with only 2 fragments and permessage-deflate enabled. The first fragment is above the threshold so it will be compressed. The second one is below the threshold and will not be compressed.

The receiving end will try to decompress both fragments causing data corruption or an error to be raised.

We should at least document this.

@Nibbler999
Copy link
Member

Choose a reason for hiding this comment

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

Good point. This should be addressed by d3fef3b

Please sign in to comment.