Skip to content

TcpDuplexConnection supports multiple subscribes - #334

Closed
lehecka wants to merge 18 commits into
rsocket:masterfrom
lehecka:multiDuplexConnection
Closed

TcpDuplexConnection supports multiple subscribes#334
lehecka wants to merge 18 commits into
rsocket:masterfrom
lehecka:multiDuplexConnection

Conversation

@lehecka

Copy link
Copy Markdown
Contributor

No description provided.

@leheckalehecka changed the title Multi duplex connectionTcpDuplexConnection supports multiple subscribesMar 31, 2017
@lehecka

Copy link
Copy Markdown
ContributorAuthor

All implementations of DuplexConnection should support repeated connecting to input/output.
This is doing it for the TcpDuplexConnection.

@facebook-github-bot

Copy link
Copy Markdown

@lehecka has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@alexmalyshevalexmalyshev left a comment

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.

lgtm. inline nits are not blocking

std::shared_ptr<Subscription> subscription) noexcept override {
if (tcpReaderWriter_) {
// no flow control at tcp level, since we can't know the size of messages
subscription->request(std::numeric_limits<size_t>::max());

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.

Is this necessary? It seems like in the next line, TcpReaderWriter::setOutputSubscription() will also do request(SIZE_T_MAX).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

good catch!
thank you

}

void requestImpl(size_t n) noexcept override {
// TcpDuplexConnection doesnt support propper flow control

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.

Nit: s|propper|proper|g

@lehecka

Copy link
Copy Markdown
ContributorAuthor

replaced with #458

Sign up for freeto 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.

3 participants

@lehecka@facebook-github-bot@alexmalyshev