Skip to content

DecoratingHttp2ConnectionEncoder.consumeReceivedSettings must not throw if de…#9343

Merged
normanmaurer merged 1 commit into4.1from
http2_connection_encoder_delegate_consumer
Jul 9, 2019
Merged

DecoratingHttp2ConnectionEncoder.consumeReceivedSettings must not throw if de…#9343
normanmaurer merged 1 commit into4.1from
http2_connection_encoder_delegate_consumer

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

…legate is instance of Http2SettingsReceivedConsumer

Motivation:

b3dba31 introduced the concept of Http2SettingsReceivedConsumer but did not correctly inplement DecoratingHttp2ConnectionEncoder.consumeReceivedSettings(...).

Modifications:

  • Add missing else around the throws
  • Add unit tests

Result:

Correctly implement DecoratingHttp2ConnectionEncoder.consumeReceivedSettings(...)

@normanmaurer
Copy link
Copy Markdown
Member Author

No idea how I missed this while reviewing the original PR :(

@normanmaurer normanmaurer added this to the 4.1.38.Final milestone Jul 9, 2019
@normanmaurer normanmaurer self-assigned this Jul 9, 2019
… if delegate is instance of Http2SettingsReceivedConsumer

Motivation:

b3dba31 introduced the concept of Http2SettingsReceivedConsumer but did not correctly inplement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...).

Modifications:

- Add missing `else` around the throws
- Add unit tests

Result:

Correctly implement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...)
@normanmaurer normanmaurer force-pushed the http2_connection_encoder_delegate_consumer branch from 9c2a800 to 81c0456 Compare July 9, 2019 09:07
@franz1981
Copy link
Copy Markdown
Contributor

LGTM
It happens sometime bud..no worries!

@normanmaurer
Copy link
Copy Markdown
Member Author

Let me just merge this one as its an easy one.

@normanmaurer normanmaurer merged commit 4312e11 into 4.1 Jul 9, 2019
@normanmaurer normanmaurer deleted the http2_connection_encoder_delegate_consumer branch July 9, 2019 12:40
normanmaurer added a commit that referenced this pull request Jul 9, 2019
… if delegate is instance of Http2SettingsReceivedConsumer (#9343)

Motivation:

b3dba31 introduced the concept of Http2SettingsReceivedConsumer but did not correctly inplement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...).

Modifications:

- Add missing `else` around the throws
- Add unit tests

Result:

Correctly implement DecoratingHttp2ConnectionEncoder.consumeRemoteSettings(...)
if (delegate instanceof Http2SettingsReceivedConsumer) {
((Http2SettingsReceivedConsumer) delegate).consumeReceivedSettings(settings);
} else {
throw new IllegalStateException("delegate " + delegate + " is not an instance of " +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be class cast exception, right?

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.

Not sure 🤔

Copy link
Copy Markdown
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Yup. That was broken 😄.

LGTM

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.

4 participants