Skip to content

Consolidate use of single ByteBufAllocator exposed from the transport #796

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

Merged
merged 3 commits into from
Apr 24, 2020

Conversation

OlegDokuka
Copy link
Member

Ensures that Transport setups and provides access to Allocator whereas the RSocketRequester/RSocketResponder may get access to set up allocator over DuplexConnection#alloc method

Signed-off-by: Oleh Dokuka [email protected]

Ensures that Transport setups and provides access to Allocator where the RSocketRequester/RSocketResponder may get access to set up allocator over DuplexConnection#alloc method

Signed-off-by: Oleh Dokuka <[email protected]>
@OlegDokuka OlegDokuka requested a review from rstoyanchev April 24, 2020 13:40
@OlegDokuka OlegDokuka changed the title refactors how ByteBufAllocator is used. Refactors how ByteBufAllocator is used Apr 24, 2020
@OlegDokuka OlegDokuka changed the title Refactors how ByteBufAllocator is used Refactors how ByteBufAllocator is used and setup Apr 24, 2020
@OlegDokuka OlegDokuka added enhancement refactorings Changes that does not affect API and behaviour and removed refactorings Changes that does not affect API and behaviour labels Apr 24, 2020
@OlegDokuka OlegDokuka added this to the 1.0 RC7 milestone Apr 24, 2020
@OlegDokuka OlegDokuka linked an issue Apr 24, 2020 that may be closed by this pull request
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Looks good.

I'm tempted to say we remove byteBufAllocator from RSocketConnector and RSocketServer altogether and clean up. The methods on RSocketFactory would be documented as "deprecated and no longer used".

It is unlikely this is used and the only potential impact would be use of heap buffers and only for things above the transport level. Given all the related effort to clean up leaks it seems reasonable to me to take this step.

WDYT?

@OlegDokuka
Copy link
Member Author

Yeah, I'm up!

@OlegDokuka OlegDokuka merged commit b3087ef into develop Apr 24, 2020
@OlegDokuka OlegDokuka deleted the enhancement/#776 branch April 24, 2020 16:50
@rstoyanchev rstoyanchev changed the title Refactors how ByteBufAllocator is used and setup Consolidate use of single ByteBufAllocator exposed from the transport Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify ByteBufAllocator usage among all the places
2 participants