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

WebSocket permessage-deflate integration #1995

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@vinniefalco
Contributor

vinniefalco commented Feb 2, 2017

Defaults to off, so rippled only blows up if you tell it to

@mellery451

LGTM

Show outdated Hide outdated doc/rippled-example.cfg
port.pmd_options.compLevel =
section.value_or("compress_level", 3);
port.pmd_options.memLevel =
section.value_or("memory_level", 4);

This comment has been minimized.

@HowardHinnant

HowardHinnant Feb 2, 2017

Contributor

There are two inconsistencies between these statements and the struct permessage_deflate declaration in beast/websocket/option.hpp, so I would like to double check that these aren't accidents:

  1. Is client_enable missing?
  2. Should the default for compLevel be 8?
@HowardHinnant

HowardHinnant Feb 2, 2017

Contributor

There are two inconsistencies between these statements and the struct permessage_deflate declaration in beast/websocket/option.hpp, so I would like to double check that these aren't accidents:

  1. Is client_enable missing?
  2. Should the default for compLevel be 8?

This comment has been minimized.

@wilsonianb

This comment has been minimized.

@vinniefalco

vinniefalco Feb 2, 2017

Contributor
  1. Nope, the Server never operates in client mode. And we don't want compression for the client we use for testing (that would just slow the tests down with no benefit)
  2. For a client, a default of 8 is okay but on a server that would crush the CPU, imagine 1,000 connections all trying to go for max compression. I made it 3 to be less resource intensive by default. Of course, if you are into pain you can always dial it up...
@vinniefalco

vinniefalco Feb 2, 2017

Contributor
  1. Nope, the Server never operates in client mode. And we don't want compression for the client we use for testing (that would just slow the tests down with no benefit)
  2. For a client, a default of 8 is okay but on a server that would crush the CPU, imagine 1,000 connections all trying to go for max compression. I made it 3 to be less resource intensive by default. Of course, if you are into pain you can always dial it up...

This comment has been minimized.

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

@wilsonianb Investigating....

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

@wilsonianb Investigating....

This comment has been minimized.

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

Looks like the range should be [0..9]
Willfix

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

Looks like the range should be [0..9]
Willfix

# offer the enabled extension in response.
#
# client_max_window_bits = [9..15]
# server_max_window_bits = [9..15]

This comment has been minimized.

@wilsonianb

wilsonianb Feb 2, 2017

Should these be [8..15]?

This parameter has a decimal integer value without leading zeroes between 8 to 15, inclusive

@wilsonianb

wilsonianb Feb 2, 2017

Should these be [8..15]?

This parameter has a decimal integer value without leading zeroes between 8 to 15, inclusive

This comment has been minimized.

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

That would be the logical thing to do but zlib defies logic:
https://github.com/madler/zlib/blob/master/deflate.c#L303

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

That would be the logical thing to do but zlib defies logic:
https://github.com/madler/zlib/blob/master/deflate.c#L303

Show outdated Hide outdated doc/rippled-example.cfg
port.pmd_options.compLevel =
section.value_or("compress_level", 3);
port.pmd_options.memLevel =
section.value_or("memory_level", 4);

This comment has been minimized.

@wilsonianb
@HowardHinnant

Just need a couple questions in a comment answered to ensure there's not an accidental oversight.

@HowardHinnant

Looks good to me!

@vinniefalco

This comment has been minimized.

Show comment
Hide comment
@vinniefalco

vinniefalco Feb 2, 2017

Contributor

Beast is missing the code that applies the compression settings to the zlib compressor! Back to the drawing board...

Contributor

vinniefalco commented Feb 2, 2017

Beast is missing the code that applies the compression settings to the zlib compressor! Back to the drawing board...

@vinniefalco vinniefalco closed this Feb 2, 2017

@vinniefalco

This comment has been minimized.

Show comment
Hide comment
@vinniefalco

vinniefalco Feb 2, 2017

Contributor

False alarm, investigating...

Contributor

vinniefalco commented Feb 2, 2017

False alarm, investigating...

@vinniefalco vinniefalco reopened this Feb 2, 2017

@wilsonianb

👍

# the least amount and 9 is the most amount. Higher levels require more
# CPU resources. Levels 1 through 3 use a fast compression algorithm,
# while levels 4 through 9 use a more compact algorithm which uses more
# CPU resources. If unspecified, a default of 3 is used.

This comment has been minimized.

@wilsonianb

wilsonianb Feb 2, 2017

Is it worth specifying the default for the other settings?

@wilsonianb

wilsonianb Feb 2, 2017

Is it worth specifying the default for the other settings?

This comment has been minimized.

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

I don't know. I rather not repeat all the information in the RFC, because its complicated and error prone. If people are hell bent on customizing it they can decipher the RFC for themselves (which specifies a default).

@vinniefalco

vinniefalco Feb 2, 2017

Contributor

I don't know. I rather not repeat all the information in the RFC, because its complicated and error prone. If people are hell bent on customizing it they can decipher the RFC for themselves (which specifies a default).

@HowardHinnant

This comment has been minimized.

Show comment
Hide comment
@HowardHinnant

HowardHinnant Feb 2, 2017

Contributor

Still 👍

Contributor

HowardHinnant commented Feb 2, 2017

Still 👍

Add permessage-deflate WebSocket support (RIPD-1409):
This also fixes a defect where the Server HTTP header was
incorrectly set in WebSocket Upgrade handshake responses.

@vinniefalco vinniefalco added the Passed label Feb 3, 2017

@ximinez

This comment has been minimized.

Show comment
Hide comment
@ximinez

ximinez Feb 8, 2017

Contributor

Merged as ce7e83f and f6a0345.

Contributor

ximinez commented Feb 8, 2017

Merged as ce7e83f and f6a0345.

@ximinez ximinez closed this Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment