Skip to content
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

Refactors the socket broadcast client #329

Merged
merged 29 commits into from
Apr 8, 2024
Merged

Conversation

arjo129
Copy link
Member

@arjo129 arjo129 commented Mar 5, 2024

This commit refactors the socket broadcast client. It switches out the underlying queue to a circular buffer, that way the memory pressure of the broadcast client remains constant without needing to do work at sporadic intervals. The new structure should hopefully also make it easier for someone to reason about the websocket layer and help us catch bugs if they exist.

I've also included an example socket client and a test server. The following plot shows memory usage after restarting the server multiple times:

example_client

This commit refactors the socket broadcast client. It switches out
the underlying queue to a circular buffer, that way the memory pressure
of the broadcast client remains constant without needing to do work at
sporadic intervals. The new structure should hopefully also make it
easier for someon to reason about the websocket layer.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
@arjo129 arjo129 marked this pull request as ready for review March 6, 2024 02:35
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
@arjo129 arjo129 requested a review from koonpeng March 21, 2024 10:16
@arjo129
Copy link
Member Author

arjo129 commented Mar 21, 2024

@aaronchongth, @koonpeng this is good to review again. I've added some integration tests.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
aaronchongth
aaronchongth previously approved these changes Mar 22, 2024
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks from me, and I saw that some files were missing EOF new lines. But overall LGTM, we can wait for an approval from KP too

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
aaronchongth
aaronchongth previously approved these changes Apr 1, 2024
@arjo129 arjo129 enabled auto-merge (squash) April 2, 2024 00:15
rmf_websocket/src/rmf_websocket/utils/RingBuffer.hpp Outdated Show resolved Hide resolved
rmf_websocket/test/test_client.cpp Outdated Show resolved Hide resolved
rmf_websocket/test/test_client.cpp Outdated Show resolved Hide resolved
Comment on lines 114 to 117
// This is a horrible piece of code and defeats
// the purpose of the tests. But unfortunately websocketpp
// does not correctly return if a send was successful or not.
// Thus packets may be lost.
Copy link

Choose a reason for hiding this comment

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

For this test, we can use a promise to wait until the reconnect happens before sending the 2nd message. Something like

const std::promise<void> on_init_promise;

auto init_function(std::promise<void>& promise) {
  ...
  promise.set_value();
}

REQUIRE_NO_THROW(on_init_promise.get_future().wait_for(1s));

// send 2nd message

Technically we can use this for test_client.cpp as well, but this requires the use of the reconnect init function, which kind of makes the 2 tests overlap.

Copy link
Member Author

Choose a reason for hiding this comment

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

After much hammering... I'm sorry to say there still are corner cases... It seems that sometimes on my machine the reconnection does not guarantee it stays... reconnected. Im trying to figure out if its abug in the logic or something else.

koonpeng
koonpeng previously approved these changes Apr 5, 2024
rmf_websocket/test/test_client.cpp Outdated Show resolved Hide resolved
rmf_websocket/test/test_client_with_update_cb.cpp Outdated Show resolved Hide resolved
koonpeng
koonpeng previously approved these changes Apr 8, 2024
@koonpeng
Copy link

koonpeng commented Apr 8, 2024

There is a minor style issue which is making the non-asan test failing.

koonpeng
koonpeng previously approved these changes Apr 8, 2024
@arjo129 arjo129 enabled auto-merge (squash) April 8, 2024 05:52
aaronchongth
aaronchongth previously approved these changes Apr 8, 2024
@arjo129 arjo129 disabled auto-merge April 8, 2024 07:21
@arjo129 arjo129 dismissed stale reviews from koonpeng and aaronchongth via 292a8a6 April 8, 2024 08:07
@arjo129 arjo129 force-pushed the arjo/fix/refactor_rmf_websocket branch from 98a29ee to 292a8a6 Compare April 8, 2024 08:07
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Tests flaky even after improvements.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Thanks @koonpeng for spotting the timing issue. It seems you were right
about the server not being cleanly closed. This commit fixes that by
cleanly closing the server in the test.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@google.com>
@arjo129 arjo129 force-pushed the arjo/fix/refactor_rmf_websocket branch from 292a8a6 to 6ae8075 Compare April 8, 2024 08:13
@arjo129 arjo129 merged commit b3e4cc1 into main Apr 8, 2024
3 of 4 checks passed
@arjo129 arjo129 deleted the arjo/fix/refactor_rmf_websocket branch April 8, 2024 08:49
arjo129 added a commit that referenced this pull request Jun 7, 2024
* Refactors the socket broadcast client

This commit refactors the socket broadcast client. It switches out
the underlying queue to a circular buffer, that way the memory pressure
of the broadcast client remains constant without needing to do work at
sporadic intervals. The new structure should hopefully also make it
easier for someon to reason about the websocket layer.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* ROS-ify logging.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Remove unessecary dependency

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Add lock

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Header guard

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Use enum for status

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Const reference to prevent copy

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Remove redundant include

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Fix move

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Move everything to one thread.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Fixed "deadlock"

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Clean up reconnection logic

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Delete move and copy constructors.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Remove FastAPI file

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* More cleanups

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Add unit tests and fix a bunch of corner cases.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Style

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address feedback

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address feedback

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address logging feedback

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address feedback

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Style

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Horrible hack to make tests always pass.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address Feedback

Tests flaky even after improvements.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Style

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Fix tests.

Thanks @koonpeng for spotting the timing issue. It seems you were right
about the server not being cleanly closed. This commit fixes that by
cleanly closing the server in the test.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Address feedback about logging.

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

* Style

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

---------

Signed-off-by: Arjo Chakravarty <arjoc@google.com>
Sign up for free to 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.

None yet

3 participants