Skip to content

Development#174

Merged
ok2c merged 2 commits intoapache:masterfrom
ok2c:development
Dec 29, 2019
Merged

Development#174
ok2c merged 2 commits intoapache:masterfrom
ok2c:development

Conversation

@ok2c
Copy link
Copy Markdown
Member

@ok2c ok2c commented Dec 28, 2019

No description provided.

@ok2c ok2c requested a review from rschmitt December 28, 2019 11:20
@ok2c
Copy link
Copy Markdown
Member Author

ok2c commented Dec 28, 2019

@rschmitt Would you like to review the change-set adding support for SETTINGS_MAX_HEADER_LIST_SIZE H2 parameter?

Comment on lines +303 to +307
listSize += header.getName().length() * 2;
if (header.getValue() != null) {
listSize += header.getValue().length() * 2;
}
listSize += 32;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like how this code duplicates logic and constants from HPackHeader. Also, why are you multiplying everything by 2?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rschmitt How would you interpret the requirement below from section 6.5.2 about length in octets? As far as I understand char representation in Java takes two octets.

uncompressed size of header fields, including the length of the
name and value **in octets** plus an overhead of 32 octets for each
header field.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am certain that the specification is referring to bytes on the wire, not the internal in-memory representation, which the peer cannot possibly know. Furthermore, due to JEP 254, modern JVMs now use one byte per character to represent most strings.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rschmitt Fair enough. Makes sense. Please review 9db8bf1

@ok2c ok2c merged commit fb0cc7f into apache:master Dec 29, 2019
@ok2c ok2c deleted the development branch December 29, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants