sciond: Change refresh mechanism to be safer #3434
Conversation
871a928 to
49d14e1
Compare
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)
a discussion (no related file):
Should we mark this as a Breaking Change? While the behavior is similar to what applications using the Refresh flag expect, it's still a behavior change.
go/lib/infra/modules/segfetcher/resolver.go, line 333 at r1 (raw file):
return false, err } return len(segs) == 0 && filtered > 0, nil
I'm missing something here, and it's probably due to my lack of familiarity with the code.
Reading this function, if len(segs) == 0, it means the slice is empty. Therefore, counting what is getting filtered out of should always return 0? In what scenario is this expected to evaluate to true?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Should we mark this as a Breaking Change? While the behavior is similar to what applications using the
Refreshflag expect, it's still a behavior change.
We can yes. Also updated the comment.
go/lib/infra/modules/segfetcher/resolver.go, line 333 at r1 (raw file):
segs.FilterSegsErr
modifies the slice inplace
Instead of deleting next query entries, just always request at the PS. This can prevent a weird interaction between concurrent requests, which would lead to 0 cached paths. Fixes scionproto#2871 Fixes scionproto#3416
49d14e1 to
09a5d56
Compare
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
go/lib/infra/modules/segfetcher/resolver.go, line 333 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
segs.FilterSegsErrmodifies the slice inplace
Ah, gotcha.
Instead of deleting next query entries, just always request at the PS.
This can prevent a weird interaction between concurrent requests, which would lead to 0 cached paths.
Marked as breaking because the behavior changes, i.e., sciond no longer deletes anything in the DB on the refresh flag.
Fixes #2871
Fixes #3416
This change is