Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

csm: fix client-to-daemon large message protocol #95

Merged
merged 5 commits into from
Jun 14, 2018

Conversation

lasch
Copy link
Member

@lasch lasch commented Jun 7, 2018

We noticed that the large-msg protocol for messages from clients to daemon was not functioning as expected. It caused the daemon to receive several invalid messages and the client to get disconnected.
This PR fixes the protocol.

@fpizzano This might be a candidate for a hotfix since it might cause trouble for anything that tries to send CSM API requests larger than 128k.

Will open an issue to add a regression test.

Copy link
Contributor

@pdlun92 pdlun92 left a comment

Choose a reason for hiding this comment

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

Passes regression, as well as the unit tests described in #96

@fpizzano
Copy link
Contributor

fpizzano commented Jun 8, 2018

For the records:

# error_inject -m 16777120 -> Work

# error_inject -m 16777121 -> Fail

@lasch
Copy link
Member Author

lasch commented Jun 12, 2018

With the latest few commits, the limit slightly shifts to 16777143.
Without these commits there was a problem with a truncated msg that would cause the receiving client to time out. Instead, the daemon shouldn't even attempt to send a msg that would get truncated.
This has been reviewed and fixed. @pdlun92 I'd suggest to retry the regression.
@fpizzano let me know if you want to experiment with the latest set of commits before we merge.

@pdlun92
Copy link
Contributor

pdlun92 commented Jun 13, 2018

@lasch Ran regression on the new code, passed again. Additionally the unit tests are still behaving as expected

@lasch lasch merged commit dea1518 into IBM:master Jun 14, 2018
pdlun92 pushed a commit to pdlun92/CAST that referenced this pull request Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants