Stream management #798
Stream management #798
Conversation
|
Failing on test broadcast, should get #797 merged first |
|
@hsanjuan I've ported most of the syncing logic in one module and make drand use this module only - there were basically three different places where we had syncing logic. |
|
TestBroadcast failing - need to merge #797 first |
|
I quite don't understand the race issue being raised by the test: It tells there are two writes at this line. However this line is only defining a function That's really weird, I haven't seen something like this before. |
AnomalRoil
force-pushed
the
fix/stream
branch
2 times, most recently
from
cfebfb3
to
360196e
Compare
| Client: bp.privGateway, | ||
| Clock: bp.opts.clock, | ||
| }) | ||
| go syncer.Run() |
There was a problem hiding this comment.
is there a reason not to have Run() as a private method that's started as a goroutine when calling NewSyncManager, vs having it as a separate method that's started as a go routine by the caller immediately after creation?
There was a problem hiding this comment.
Good point, I don't really have any preference.
There was a problem hiding this comment.
At least like that, it's explicit that it's running in its own go routine? If you feel I should change it, I can change it. Let me know.
|
@willscott Yes, it's implicit: a drand daemon can have multiple beacon processes (with different beacon IDs), each registering a beacon handler, which has a chainStore, which has a SyncManager. |
| func (s *SyncManager) tryNode(global context.Context, upTo uint64, n net.Peer) bool { | ||
| // we put a cancel to still keep the global context open but stop with this | ||
| // peer if things go sideway | ||
| cnode, cancel := context.WithCancel(global) |
There was a problem hiding this comment.
Should we have a context with a timeout here, or do we rely on the GRPC client to timeout?
There was a problem hiding this comment.
I think we started trying to plumb life-cycles through a bit more previously, but didn't really get them complete so I don't think we need to require that as part of this PR either.
Adding earlier exit condition Correct logging Skip own node for sync
Updating protoc too
This PR revisits the stream mode and makes sure the go routines are exited from any errors. Fixes #795