fix: don't panic when processing unordered operations in scheduler #130
Conversation
When the scheduler processes multiple operations, it stores then
in a slice according to their `Index` field:
```go
store[op.Id][op.Index] = op
```
To ensure the slice has the capacity to store the item, there's an
allocation block, right before:
```go
if len(store[op.Id]) <= op.Index {
store[op.Id] = append(store[op.Id], op)
}
```
However, the code above panics with "index out of range" if the delta
between the slice length and the operation index is greater than 1 --
for instance, if the slice is empty and the first operation has index 2.
To fix it, we must perform the `append` multiple times:
```go
for len(store[op.Id]) <= op.Index {
store[op.Id] = append(store[op.Id], op)
}
```
| @@ -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()) { | |||
There was a problem hiding this comment.
So is it fine to keep appending the same op if the index is larger?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
in line 94, we do:
tw.store[op.Id][op.Index.Int64()] = opwhich 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.
There was a problem hiding this comment.
ah i see, i was wondering what tw.store[op.Id][op.Index.Int64()] = op was doing.
gustavogama-cll
changed the title
gustavogama-cll
deleted the
fix-dont-panic-unordered-operations-processed-scheduler
branch
When the scheduler processes multiple operations, it stores then in a slice according to their
Indexfield:To ensure the slice has the capacity to store the item, there's an allocation block, right before:
However, the code above panics with "index out of range" if the delta between the slice length and the operation index is greater than 1 -- for instance, if the slice is empty and the first operation has index 2. To fix it, we must perform the
appendmultiple times:JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1528