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.

Sign up for GitHub

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

Amend timeout to handle in-flight packets. #3923

Merged
merged 8 commits into from
Jul 6, 2023
Merged

Amend timeout to handle in-flight packets. #3923

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65d290f
Amend timeout to handle in-flight packets.
DimitrisJim Jun 22, 2023
b601808
Update timeout handler per spec.
DimitrisJim Jun 28, 2023
e15d3af
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jun 29, 2023
9adb05a
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jul 5, 2023
e74bced
Update tests to test for toggling of flush status.
DimitrisJim Jul 5, 2023
2e73fd2
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
crodriguezvega Jul 6, 2023
d3a60ee
Merge branch '04-channel-upgrades' into jim/3879-packet-flushing-timeout
DimitrisJim Jul 6, 2023
91dc1ad
Fix small typos in docstring.
DimitrisJim Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions modules/core/04-channel/keeper/timeout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,14 @@ func (k Keeper) TimeoutExecuted(

k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
Copy link
Contributor Author

@DimitrisJim DimitrisJim Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

P.s, these could also be:

switch channel.Ordering {
case types.ORDERED:
	channel.State = types.CLOSED
	channel.FlushStatus = types.NOTINFLUSH
case types.UNORDERED:
	if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) {
		channel.FlushStatus = types.FLUSHCOMPLETE
	}
}
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)

and, considering ORDERED_ALLOW_TIMEOUT, this could be amended in the future to possible be case types.UNORDERED, types.ORDERED_ALLOW_TIMEOUT since it looks like it would require similar handling.

Happy to leave as is but dropping this sionce I noticed it.

Copy link
Contributor

@charleenfei charleenfei Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i think the switch statement is nice for readability!

but i guess we probably need to check if channel.FlushStatus == types.FLUSHING && !k.hasInflightPackets(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) in both cases rather than just in the UNORDERED case, and then basically if it's ORDERED set to CLOSED/NOTINFLUSH, and do the ChannelClosedEvent in that case, and if it's UNORDERED set to FLUSHCOMPLETE. but i'm not super opinionated on which style we go with.

channel.FlushStatus = types.FLUSHCOMPLETE
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
}

if channel.Ordering == types.ORDERED {
channel.State = types.CLOSED
channel.FlushStatus = types.NOTINFLUSH
k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel)
}

Expand Down