Fix issues with HTTP subscriptions #356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Modern LLVM errors on this deprected use of the keyword. Signed-off-by: Alexander Krimm <[email protected]>
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
commented
Nov 6, 2024
Comment on lines
-328
to
-337
| auto redirect_get = [&client](auto url, auto headers) { | ||
| for (;;) { | ||
| auto result = client.Get(url, headers); | ||
| if (!result) return result; | ||
|
|
||
| if (result->status >= 300 && result->status < 400) { | ||
| url = httplib::detail::decode_url(result.value().get_header_value("location"), true); | ||
| } else { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
This was not needed, since httplib resolves redirects internally with the set_follow_location(true) setting which is applied above
wirew0rm
commented
Nov 6, 2024
| @@ -393,10 +389,9 @@ class RestBackend : public Mode { | |||
| _connectionUpdaterThread = std::jthread([this](std::stop_token stop_token) { | |||
| thread::setThreadName("RestBackend updater thread"); | |||
| while (!stop_token.stop_requested()) { | |||
| std::this_thread::sleep_for(100ms); | |||
| std::this_thread::sleep_for(10ms); | |||
There was a problem hiding this comment.
This was the actual rate limit of 1/100ms=1Hz on the updates coming from the broker. TBD if it is needed at all or should be even lower/only applied if there are no connections to poll on.
| @@ -453,7 +444,7 @@ class RestBackend : public Mode { | |||
| if (events & ZMQ_POLLIN) { | |||
| auto *currentConnection = connections[i]; | |||
| auto connectionLock = currentConnection->writeLock(); | |||
| if (auto responseMessage = zmq::receive<mdp::MessageFormat::WithoutSourceId>(currentConnection->notificationSubscriptionSocket)) { | |||
| while (auto responseMessage = zmq::receive<mdp::MessageFormat::WithoutSourceId>(currentConnection->notificationSubscriptionSocket)) { | |||
There was a problem hiding this comment.
This change lets the client handle all available notifications for a connection at once, allowing it to catch up. One open question I have here is whether this is safe, as it seems like zmq::receive drops everything already received frames when it cannot get all required frames of a message. Basically the question is if Messages are sent atomically or if we could get partial messages here.
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
had a problem deploying
to
configure coverage
GitHub Actions
Failure
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
Changes the RestClient to internally track the last received update index instead of just blindly always requesting the latest update. This prevents duplicate notifications and allows catching up and blocking on not yet arrived updates. For emscripten this needs a bit of trickery because the api of emscripten_fetch does not provide a way to access the final redirected URL, so we have to call out to javascript to retrieve it. Signed-off-by: Alexander Krimm <[email protected]>
The poll loop that handles the messages from the broker had a 100ms sleep built-in and only handled only one update per iteration and connection. This lead to updates being acumulated in the RestBackend's subscription socket, whenever the underlying property would update with more than 10Hz. This commit reduces the sleep between poll loops to 10ms and handles all notifications for all connections at once, allowing the RestBackend to catch up on a burst of notifications. smaller changes: - make http headers case-insensitive - general code-cleanup Signed-off-by: Alexander Krimm <[email protected]>
Fix the unittests which do not corretly handle the LongPollingIdx parameter. Signed-off-by: Alexander Krimm <[email protected]>
wirew0rm
force-pushed
the
restClientLongPollingIdx
branch
from
3f4f431 to
4e1d52e
Compare
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
wirew0rm
temporarily deployed
to
configure coverage
GitHub Actions
Inactive
|
RalphSteinhagen
approved these changes
Nov 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes some http subscription issues, the most obvious a latency buildup, whenever the notifiation rate was more than 10Hz. I also noticed that the RestClients did not track the last received LongPollingIdx which makes it impossible for them to catch up or even detect missing updates.
See commit messages for more details.
I'll push a draft PR to opendigitizer based on these changes, which contains a utilitiy to analyze end to end processing latency analysis tools.
Also had to remove some
templatekeywords to get my local emscripten build compiling again.