Conversation
peterargue
left a comment
There was a problem hiding this comment.
this is looking great! could use some comments, particularly in common.go
huitseeker
left a comment
There was a problem hiding this comment.
This looks awesome! Thank you so much for documenting the channel logic in RunComponent and the ComponentManager's Start, and the ErrorHandlingResult in particular is 👌
I suspect a "further work" task may be to add a runnable example.
|
|
||
| func NewSignaler(errors chan<- error) *Signaler { | ||
| return &Signaler{errors} | ||
| func NewSignaler() (*Signaler, <-chan error) { |
| // the One True Way of getting a SignalerContext | ||
| func WithSignaler(ctx context.Context, sig *Signaler) SignalerContext { | ||
| return signalerCtxt{ctx, sig} | ||
| func WithSignaler(parent context.Context) (SignalerContext, <-chan error) { |
There was a problem hiding this comment.
👍 I like that this is consistent with other With* context methods
module/component/component.go
Outdated
|
|
||
| type ReadyFunc func() | ||
|
|
||
| // ComponentWorker represents a worker routine of a component |
There was a problem hiding this comment.
can you add some more details here so implementors know how this should be used. I think we've discussed adding general worker routines as well as subcomponent startup logic and kicking off shutdown handlers.
| defer close(c.done) | ||
|
|
||
| // throw fatal error during startup | ||
| ctx.Throw(ErrFatal) |
There was a problem hiding this comment.
what happens if this is thrown outside of the goroutine? that would exit the routine calling RunComponent right? which would bypass the error handler. maybe we need to either call the start method in it's own goroutine, or explicitly call out in the comments for RunComponent that throw should only be called within a subroutine
|
@peterargue addrssed comments |
|
bors merge |
1355: [Network] Middleware component r=smnzhu a=smnzhu * Refactor middleware to implement the `Component` interface. * Introduces new `ComponentManager` struct to help implement `Component` interface * Various refactoring in network layer and scaffold to enable the changes above. ### TODO - [x] As mentioned in #1167 (comment), we should probably explicitly throw an error when `Start` is called multiple times, instead of simply ignoring subsequent calls - [x] Update the godoc for Startable to reflect this - [x] Add tests for ComponentManager Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com>
|
Build failed: |
|
bors retry |
Componentinterface.ComponentManagerstruct to help implementComponentinterfaceTODO
Startis called multiple times, instead of simply ignoring subsequent calls