Skip to content

Conversation

@bhandras
Copy link
Member

No description provided.

@rockstardev
Copy link
Contributor

Thanks for quick turnaround on this @bhandras, now I only hope review from @guggero @carlaKC and merge + release will be as fast ;)

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Just some nits!

// try to re-attempt block epoch subscription.
select {
case <-time.After(50 * time.Millisecond):
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a log here that we're waiting for the chain notifier? Otherwise loop will just be hanging and user may not know why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Logging sounds good to me. And maybe increase the delay to 500ms to not spam lnd too much? It's got enough on its plate at this moment of startup anyway 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the 50ms was a bit overkill :) Log added.

Comment on lines +80 to +86
return err
}

break
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to break? Couldn't we just return err here? Either it's nil or it's a non-"process of starting" error.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function continues below. So getting here means we've succeeded with the connection.

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!
Just one nit, otherwise LGTM 💯

Comment on lines +80 to +86
return err
}

break
Copy link
Contributor

Choose a reason for hiding this comment

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

The function continues below. So getting here means we've succeeded with the connection.

// try to re-attempt block epoch subscription.
select {
case <-time.After(50 * time.Millisecond):
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Logging sounds good to me. And maybe increase the delay to 500ms to not spam lnd too much? It's got enough on its plate at this moment of startup anyway 😂

@bhandras bhandras marked this pull request as ready for review April 29, 2021 09:39
@bhandras bhandras force-pushed the chainnoitifer_fix branch from 89638eb to a0b67da Compare April 29, 2021 09:39
@bhandras bhandras requested a review from carlaKC April 29, 2021 09:39
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

New record for fastest issue fix in loop! 🏁

LGTM

if strings.Contains(err.Error(),
"in the process of starting") {

log.Warnf("LND chain notifier server not " +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could just be Warn, but don't push again for this!

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.

Loop occasionally fails to start if LND chain notifier RPC is still in the process of starting

4 participants