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

Infra: Error out if initial sciond connect fails #2790

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Jun 21, 2019

Currently, infraenv.NewRouter returns a router with a nil sciond
connection, if every attempt fails to connect to sciond.

This happens because the err variable is shadowed, and thus never
set in the outer scope.


This change is Reviewable

Currently, `infraenv.NewRouter` returns a router with a nil sciond
connection, if every attempt fails to connect to sciond.

This happens because the `err` variable is shadowed, and thus never
set in the outer scope.
@oncilla oncilla added the bug Something isn't working label Jun 21, 2019
@oncilla oncilla added this to the Q2S3 milestone Jun 21, 2019
@oncilla oncilla requested a review from scrye June 21, 2019 12:54
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)


go/lib/infra/infraenv/infraenv.go, line 309 at r1 (raw file):

		sciondConn, err := sciond.NewService(sd.Path, true).Connect()
		if err == nil {
			router = &snet.BaseRouter{

What do you think about simplifying this further, to something like:

// NewRouter constructs a path router for paths starting from localIA.
func NewRouter(localIA addr.IA, sd env.SciondClient) (snet.Router, error) {
        ticker := time.NewTicker(time.Second)
        timer := time.NewTimer(sd.InitialConnectPeriod.Duration)
        defer ticker.Stop()
        defer timer.Stop()
        // XXX(roosd): Initial retrying is implemented here temporarily.
        // In https://github.com/scionproto/scion/issues/1974 this will be
        // done transparently and pushed to snet.NewNetwork.
        for {
                sciondConn, err := sciond.NewService(sd.Path, true).Connect()
                if err != nil {
                        select {
                        case <-ticker.C:
                        case <-timer.C:
                                return nil, common.NewBasicError("Timed out during initial sciond connect", err)
                        }
                } else {
                        return &snet.BaseRouter{
                                IA: localIA,
                                PathResolver: pathmgr.New(
                                        sciondConn,
                                        pathmgr.Timers{
                                                NormalRefire: time.Minute,
                                                ErrorRefire:  3 * time.Second,
                                        },
                                        log.Root(),
                                ),
                        }, nil
                }
        }
}

@oncilla oncilla merged commit a261159 into scionproto:master Jun 21, 2019
@oncilla oncilla deleted the pub-sciond-reconnect branch September 3, 2020 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants