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

fix: Add exceptions for edge cases for best effort responses. #3155

Merged

Conversation

stiegerc
Copy link
Contributor

@stiegerc stiegerc commented Dec 12, 2024

For best effort responses, it is expected that sometimes a second response arrives late. These cases are filtered by checking against the callback when inducting into the replicated state, but the canister could have been stopped or even removed in the mean time in which case we don't even make it to this check. Therefore these cases must be filtered elsewhere since they are expected and not critical errors.

@github-actions github-actions bot added the fix label Dec 12, 2024
@stiegerc stiegerc marked this pull request as ready for review December 12, 2024 15:49
@stiegerc stiegerc requested a review from a team as a code owner December 12, 2024 15:49
@derlerd-dfinity
Copy link
Contributor

Should we have a test for this case as well?

@stiegerc
Copy link
Contributor Author

stiegerc commented Dec 12, 2024

Should we have a test for this case as well?

I can add something for this specifically but you could argue that we don't have any unit tests for best effort anything at the moment. The pragmatic approach would seem to be adding tests for exceptions like this one rather than duplicating everything and then also higher level tests such as state machine tests (which is how I found this bug).

@alin-at-dfinity
Copy link
Contributor

alin-at-dfinity commented Dec 13, 2024

I can add something for this specifically but you could argue that we don't have any unit tests for best effort anything at the moment.

Indeed, we do not have any tests for best-effort messages in Message Routing proper. We do have quite a few of them lower down, in ic_replicated_state. I guess it may make sense to test most (all?) of the potential combinations (request / response, best-effort / guaranteed, canister running / stopping / stopped / deleted / no matching call context / whatever) as protection against regression. It could just be a bunch of for loops and a huge match statement.

Edit: Not necessarily for this PR, just stating my opinion.

@stiegerc
Copy link
Contributor Author

There is a bit of confusion whether we should use best effort or best-effort. I went with best-effort since that occurs more often by a factor of 3 or so. I can drive-by standardize that in this PR if you want.

rs/replicated_state/src/canister_state/tests.rs Outdated Show resolved Hide resolved
rs/replicated_state/tests/replicated_state.rs Outdated Show resolved Hide resolved
rs/messaging/src/routing/stream_handler.rs Outdated Show resolved Hide resolved
@stiegerc stiegerc added this pull request to the merge queue Dec 16, 2024
Merged via the queue into master with commit a68611e Dec 16, 2024
25 checks passed
@stiegerc stiegerc deleted the add_exceptions_for_stopped_and_not_found_for_best_effort branch December 16, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants