Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Network] Middleware component #1355

Merged
merged 53 commits into from Oct 12, 2021
Merged

[Network] Middleware component #1355

merged 53 commits into from Oct 12, 2021

Conversation

synzhu
Copy link
Contributor

@synzhu synzhu commented Sep 26, 2021

  • 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

  • As mentioned in [FLIP] Component API #1167 (comment), we should probably explicitly throw an error when Start is called multiple times, instead of simply ignoring subsequent calls
  • Update the godoc for Startable to reflect this
  • Add tests for ComponentManager

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is looking great! could use some comments, particularly in common.go

network/p2p/network.go Outdated Show resolved Hide resolved
@peterargue peterargue mentioned this pull request Sep 28, 2021
1 task
Base automatically changed from smnzhu/error-handling to master September 29, 2021 04:55
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is cleaner


// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like that this is consistent with other With* context methods


type ReadyFunc func()

// ComponentWorker represents a worker routine of a component
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

@peterargue peterargue Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

network/p2p/network.go Show resolved Hide resolved
@synzhu
Copy link
Contributor Author

synzhu commented Oct 9, 2021

@peterargue addrssed comments

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome work!

@synzhu
Copy link
Contributor Author

synzhu commented Oct 12, 2021

bors merge

bors bot added a commit that referenced this pull request Oct 12, 2021
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>
@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

Build failed:

@synzhu
Copy link
Contributor Author

synzhu commented Oct 12, 2021

bors retry

@bors
Copy link
Contributor

bors bot commented Oct 12, 2021

@bors bors bot merged commit 361ba2d into master Oct 12, 2021
@bors bors bot deleted the smnzhu/middleware-component branch October 12, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants