threads: fetch all unknown records #132
Conversation
Signed-off-by: jsign <[email protected]>
Signed-off-by: jsign <[email protected]>
Signed-off-by: jsign <[email protected]>
Signed-off-by: jsign <[email protected]>
Signed-off-by: jsign <[email protected]>
Signed-off-by: jsign <[email protected]>
There was a problem hiding this comment.
These changes seem to completely fix the problem in the folder-sync app regarding can't update instance that doesn't exit, which was the consecuence of missing records.
In particular, save events for instances were being processed without receiving the create event that should prior.
|
This was an important step to have some correctness in the underlying working of threads. TBH, I tried to make those changes in this PR, but they're a bit more complicated because I think I should move some code to other places (so to no double lock the same mutex, etc). Prefer to keep this PR as short as possible. |
Signed-off-by: jsign <[email protected]>
|
|
||
| // Fix concurrency: adding head without having the block guarantee | ||
| if err := s.threads.store.AddLog(req.ThreadID.ID, lg); err != nil { | ||
| return nil, status.Error(codes.Internal, err.Error()) | ||
| } | ||
|
|
||
| go func() { | ||
| func() { | ||
| // Get log records for this new log | ||
| recs, err := s.getRecords( | ||
| s.threads.ctx, |
There was a problem hiding this comment.
Remembering the logic... you should only receive a log here if you got pushed a record without having the log, at which point you'd request it. However, that doesn't stand as a good enough safety measure. There's a TODO in the method comment mentioning that we ought to only overwrite the log if it comes from the owner. Maybe that only applies to the addresses: update the addresses if the sender is the log owner. I may have left the TODO here because there isn't currently a way to tell if the sender is the owner (also related to the verification TODO). Each log can be signed with the log's private key if the sender has it.
Agreed, updating the head should probably not be done here.
| t.pullLock.Lock() | ||
| defer t.pullLock.Unlock() | ||
| // Wait for all thread pulls to finish | ||
| for _, semaph := range t.pullLocks { | ||
| semaph <- struct{}{} | ||
| } |
|
|
||
| // PullThread for new records. | ||
| // Logs owned by this host are traversed locally. |
There was a problem hiding this comment.
Not related to this PR, but this comment must be outdated / relic from the old pull functionality.
Replied to all comments. I'll merge tomorrow. Feel free to create issues for many points of discussion. Will be nice to have them written before forgetting since all are quite important/interesting. |
|
I’ve made a note to create some issues from the discussions. Will do when I’m back on! |
Fixes #128
Apart from that main fix, I'll add some further comments with some considerations that I'd like @sanderpick opinion.