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

fix: don't panic when processing unordered operations in scheduler #130

Merged
merged 1 commit into from
Feb 4, 2025
Merged

fix: don't panic when processing unordered operations in scheduler #130

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion pkg/timelock/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (tw *scheduler) runScheduler(ctx context.Context) <-chan struct{} {

case op := <-tw.add:
tw.mu.Lock()
if len(tw.store[op.Id]) <= int(op.Index.Int64()) {
for len(tw.store[op.Id]) <= int(op.Index.Int64()) {
Copy link

Choose a reason for hiding this comment

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

So is it fine to keep appending the same op if the index is larger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we append while the index is smaller or the same. In order words, if we're supposed to process an operation with index 4, we make sure indexes 0, 1, 2 and 3 exist.

But I just realized there's a potential problem with this fix, in case we have many operations in the same event and they arrive out-of-order. I tried to adapt the existing code without changing it too much but now I don't think that's possible.

I'll push an update shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... after some more thinking, I believe the current implementation will work fine. I thought for a second it might overwrite valid entries but upon further review, that doesn't seem to be the case.

So, to sum it up once more: given a series of operations with the same id and indices 0, 1, 2 and 3, if they arrive in the order 0, 3, 2, 1, this implementation should do the right thing: when processing index 3, it backfills the slice, making sure indices 1 and 2 are temporarily populated with op 3. The previous implementation would panic because it would append just one item to the slice and then panic with index out of range.

Copy link

Choose a reason for hiding this comment

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

making sure indices 1 and 2 are temporarily populated with op 3

is this true? lets say indices arrive in [3,2,1,0] ,
we first populate

tw.store[op.Id] = [3,3,3,3]

then when we process index 2,
we will skip because if len(tw.store[op.Id]) <= int(op.Index.Int64()) { is false?

Is that what we want ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in line 94, we do:

tw.store[op.Id][op.Index.Int64()] = op

which leaves us with [3,3,2,3].

I'm not 100% sure it's what we want, but it's similar to what we had before. The only difference is we now populate more than just the previous index and thus avoid the panic we got in Jaden's tests.

Copy link

Choose a reason for hiding this comment

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

ah i see, i was wondering what tw.store[op.Id][op.Index.Int64()] = op was doing.

tw.store[op.Id] = append(tw.store[op.Id], op)
}
tw.store[op.Id][op.Index.Int64()] = op
Expand Down
Loading