Gap fill with wrong seq num & not inflating correctly if end seq is 0 #83
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
douglasmuraoka
changed the title
douglasmuraoka
commented
Dec 10, 2023
| @@ -72,23 +72,88 @@ test('end to end logon', async () => { | |||
| test('session send resendRequest when logged on', async () => { | |||
| const runner: SkeletonRunner = new SkeletonRunner(experiment, 2) | |||
| const factory = experiment.client.config.factory | |||
| const resend = factory?.resendRequest(1, 2) | |||
| const resend = factory?.resendRequest(1, 1) | |||
There was a problem hiding this comment.
In this scenario, we've only sent the login message so far, so there's no reason for it to have end seq num = 2 (unless it is 2 on purpose, for some reason)...
| expect(serverResets).toEqual(0) | ||
| }) | ||
|
|
||
| test('session send resendRequest with endSeqNo = 0 when logged on', async () => { |
There was a problem hiding this comment.
This test must have the same behavior as above, as end seq num = 0 should be resolved to 1 (which is the last seq num sent).
|
@TimelordUK let me know if it would be better to split these into smaller PRs so we discuss them individually. |
|
This is great work and really very much appreciated. Replay has been tricky to get right and you have really moved things forward. I may add some more tests later. I am checking now and will merge very soon. Thanks a lot for another good PR |
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.
Found three minor issues.
One in
sequenceResetGap, where the gap fill seq num was set withnewSeq(should bestartGap, because the seq num represents the start of the gap).Another issue is when the
endSeqNois0. The existing code doesn't handle this scenario, and no gap fills are created. In this case, when the end seq no is 0, we should consider it as the latest seq num sent to the other peer (that's why I get it fromthis.sessionState.lastSentSeqNum(), but let me know if this inaccurate).And the last issue is when we resend messages with zero messages retrieved from the store. In a situation where the only message sent to the other peer is
login(seq num=1), for example, the store will be empty. But still, we have to generate a gap fill from1to1to respond to theResendRequestmessage. The current code doesn't return anything (empty array of messages to be sent to peer).