fix: IsAuthenticated cancellation races #421
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
3v1n0
force-pushed
the
isauthenticated-race-fixes
branch
3 times, most recently
from
35187a9
to
f635b30
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 84.57% 84.74% +0.17%
==========================================
Files 77 77
Lines 6731 6780 +49
Branches 75 75
==========================================
+ Hits 5693 5746 +53
+ Misses 728 725 -3
+ Partials 310 309 -1 ☔ View full report in Codecov by Sentry. |
…ures Use instead a native signal so that we don't risk sending it back to the default implementation, causing unwanted behaviors
3v1n0
force-pushed
the
isauthenticated-race-fixes
branch
3 times, most recently
from
aa7f03c
to
51249c5
Compare
didrocks
approved these changes
Jul 9, 2024
The isAuthenticatedCalls map is shared between the various calls and so we should not access to it without locking the mutex, however we defined the local context just before, so there's no need to use it though the map
…function When modifying the content of the model on deferred function, we need to be sure that we're actually modifying the model that we're returning, not the value that is stored at the place that the function-level model pointer is pointing to, since we've returned a copy of it. Thus we were not really changing what's passed to the main model and then not really updating it
In case we're still in the process of cancelling the authentication we should not continue with the current action until we're notified that the cancellation is completed. This is needed because otherwise we may end up into this situation: - reselectAuthMode - AuthModeSelected - ... - startAuthentication - isAuthenticatedResultReceived: cancelled Or a bit less extreme, but still getting the cancelled event while we're selecting the authentication mode. And this mis-order causes troubles especially when passing the data to gdm. To avoid this, add a tracker to wait for the authentication cancellation to be completed and only once we've received a response we can proceed with next actions. Due to the blocking nature of channels we need to use more async functions now and add a few synchronization points In particular we need to add some sleeps to workaround two main issues: 1. Wait for the IsAuthenticated to be delivered before cancelling it 2. Wait to start IsAuthentication requests after cancellation error # 1. Sleep between the request and its cancellation When quickly calling IsAuthenticated followed by context cancellation, we may end up in a situation in which we cancel the request before we've actually placed one on the broker. This is due to the fact that the dbus request may take some time to be delivered and being stored by a remote broker, while the cancellation may happen very quickly. So, to prevent this, in case the context has been cancelled, let's wait "a bit" before sending the cancellation request, so that we can be sure that the `IsAuthenticate` request has been delivered before. This is far from being nice, but it's the best way to achieve this result given the API constraints that we have, and the fact that go-dbus can't advertise us if a message has been delivered, without waiting for the whole result. See example log of this failure at: https://www.pastery.net/afkmuy/ # 2. Sleep after receiving the cancelled error The IsAuthentication request on client side may happen even while we're still cancelling the previous one, this is because the client receives the cancellation error as just after the calling context is cancelled, and not when the server side has actually completed and returned the request. This is due to the nature of the grpc.ClientConnInterface used, and there's no simple way to have this information client-side without rewriting the client low level implementation. It's very easy to reproduce this case by just increasing the sleeping time we added to fix previous point, making the cancellation to be slower than actually is. To workaround this, wait some reasonable extra time before delivering the cancelled event, so that we can be confident enough that the server side has received and cancelled/completed the previous request. This is far from optimal, but we have no other way at client side to know that the server has finished handling the all the previous requests.
This was failing in previous versions because of the races fixed in this branch, so ensure we won't fail anymore
Example broker did support qrcode's code regeneration, but this didn't lead to new content or code, making testing its actual functionality harder. So, each time the qrcode is selected we now loop through different (but predictable) URIs and increase the code value. As per this regenerate golden files so that the regeneration is tracked Add support for `user-integration-static-qrcode` users so that we don't have to track the content changes in brute-force tests where we don't have a clear number of regeneration events and we don't care about such changes.
3v1n0
force-pushed
the
isauthenticated-race-fixes
branch
from
87b5750
to
f0d159d
Compare
Yeah, some parts are, the fact that we've to wait for some unknown amount of time doesn't make me fully happy, but we still had something similar before and at least now everything is in a single place. Meanwhile, rebased and dropped the already reverted commits (so no-change force-push in the end). |
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.
Both on the client side and the server side we have some troubles in handling repeated
IsAuthenticatedrequests followed by cancellations.In particular:
See commits fixing (or workarounding) each issue here for deeper explanations and the rationale for the fix or workaround in place.
UDENG-3417