Skip to content

Conversation

@maskit
Copy link
Member

@maskit maskit commented Apr 20, 2017

This closes #1591.

There are two commits. The first one is a bug fix for the table size update issue. The second commit adds an implementation limit to avoid abusing. The maximum table size is advertised by clients, but we can use any value lower than the value. The value 64k is came from major client implementations.
https://bugzilla.mozilla.org/show_bug.cgi?id=1296280
https://bugs.chromium.org/p/chromium/issues/detail?id=642784

I will squash the commits before merging.

@maskit maskit added the HTTP/2 label Apr 20, 2017
@maskit maskit added this to the 7.1.0 milestone Apr 20, 2017
@maskit maskit self-assigned this Apr 20, 2017
@maskit maskit requested review from bryancall and masaori335 April 20, 2017 02:40
@atsci
Copy link

atsci commented Apr 20, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/273/

@atsci
Copy link

atsci commented Apr 20, 2017

@atsci
Copy link

atsci commented Apr 20, 2017

@atsci
Copy link

atsci commented Apr 20, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/384/

@atsci
Copy link

atsci commented Apr 20, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1954/

@atsci
Copy link

atsci commented Apr 20, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1846/

int32_t maximum_table_size)
{
// Limit the maximum table size to 64kB, which is the size advertised by major clients
maximum_table_size = min(maximum_table_size, 64 * 1024);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to make 64 * 1024 a constant variable.

@atsci
Copy link

atsci commented Apr 20, 2017

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/517/

@atsci
Copy link

atsci commented Apr 21, 2017

@atsci
Copy link

atsci commented Apr 21, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/283/

@atsci
Copy link

atsci commented Apr 21, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1964/

@atsci
Copy link

atsci commented Apr 21, 2017

@atsci
Copy link

atsci commented Apr 21, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/394/

@atsci
Copy link

atsci commented Apr 21, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1856/

@atsci
Copy link

atsci commented Apr 21, 2017

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/527/

@masaori335
Copy link
Contributor

I'm OK basically but atsci looks like still angry:( At least, clang-format should be passed.

@atsci
Copy link

atsci commented Apr 22, 2017

@atsci
Copy link

atsci commented Apr 22, 2017

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/295/

@atsci
Copy link

atsci commented Apr 22, 2017

@atsci
Copy link

atsci commented Apr 22, 2017

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1976/

@atsci
Copy link

atsci commented Apr 22, 2017

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/407/

@atsci
Copy link

atsci commented Apr 22, 2017

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1868/

@atsci
Copy link

atsci commented Apr 22, 2017

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/540/

@maskit maskit merged commit 60471c7 into apache:master Apr 24, 2017
@zwoop
Copy link
Contributor

zwoop commented Apr 24, 2017

I've cherry-picked this to 7.1.x branch as well.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advertised header table size is not used

4 participants