Skip to content

Ensure ByteToMessageDecoder.Cumulator implementations always release …#8325

Merged
normanmaurer merged 1 commit into4.1from
cumulator_always_release_in
Sep 27, 2018
Merged

Ensure ByteToMessageDecoder.Cumulator implementations always release …#8325
normanmaurer merged 1 commit into4.1from
cumulator_always_release_in

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

…in buffer.

Motivation:

We need to ensure the Cumulator always releases the input buffer if it can not take over the ownership of it as otherwise it may leak.

Modifications:

  • Correctly ensure the buffer is always released.
  • Add unit tests.

Result:

Ensure buffer is always released.

Copy link
Copy Markdown
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

@normanmaurer I noticed couple of small things...

composite.addComponent(true, cumulation);
}
composite.addComponent(true, in);
releaseIn = false;
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.

why not just set in to null?

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.

good point... lets do it.

buffer.writeBytes(in);
return buffer;
} finally {
// We most release in in all cases as otherwise it may produce a leak if writeBytes(...) throw
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.

typo most -> must

…in buffer.

Motivation:

We need to ensure the Cumulator always releases the input buffer if it can not take over the ownership of it as otherwise it may leak.

Modifications:

- Correctly ensure the buffer is always released.
- Add unit tests.

Result:

Ensure buffer is always released.
@normanmaurer normanmaurer force-pushed the cumulator_always_release_in branch from abe5ef3 to 61cf9d9 Compare September 26, 2018 18:25
@normanmaurer
Copy link
Copy Markdown
Member Author

@njhill thanks for the review.. done :)

@normanmaurer normanmaurer merged commit c546ab2 into 4.1 Sep 27, 2018
@normanmaurer normanmaurer deleted the cumulator_always_release_in branch September 27, 2018 05:38
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.

3 participants