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

feat: reconnect immediately on first retry #338

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Conversation

im-adithya
Copy link
Member

Fixes #297

Is this what we want here @rolznz?

@im-adithya im-adithya requested a review from rolznz July 25, 2024 14:02
@rolznz
Copy link
Contributor

rolznz commented Jul 25, 2024

@im-adithya please re-read the issue and think through it. If you don't understand it, please do ask before writing code

@im-adithya im-adithya changed the title chore: don't wait 10 seconds if it fails once feat: reconnect immediately on first retry Jul 25, 2024
@im-adithya
Copy link
Member Author

@rolznz can you re-review?

@rolznz rolznz added this to the v1.4.0 milestone Jul 31, 2024
service/start.go Outdated
@@ -63,6 +64,8 @@ func (svc *service) startNostr(ctx context.Context, encryptionKey string) error
if contextCancelled {
break
}
} else if i > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this if-else with checking the i value makes things over-complicated. I don't think we need to check the iteration at all now since we now control whether we back off or not.

service/start.go Outdated
@@ -46,10 +46,11 @@ func (svc *service) startNostr(ctx context.Context, encryptionKey string) error
defer svc.wg.Done()
//Start infinite loop which will be only broken by canceling ctx (SIGINT)
var relay *nostr.Relay
immediateRetry := false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be easier to understand to flip this and call it waitToReconnectSeconds, and use a number instead of a boolean, so that later we are able to easily adjust the algorithm (e.g. do an exponential back off algorithm)

service/start.go Outdated

for i := 0; ; i++ {
// wait for a delay before retrying except on first iteration
if i > 0 {
if i > 0 && !immediateRetry {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the iteration check here and do if waitToReconnectSeconds > 0

@rolznz rolznz removed this from the v1.4.0 milestone Jul 31, 2024
@rolznz
Copy link
Contributor

rolznz commented Aug 2, 2024

@im-adithya can you address the feedback?

@rolznz rolznz added this to the v1.4.1 milestone Aug 2, 2024
@im-adithya
Copy link
Member Author

Ready now @rolznz!

Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

@rolznz rolznz merged commit 0327150 into master Aug 2, 2024
8 checks passed
@rolznz rolznz deleted the task-connect-relay branch August 2, 2024 15:10
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.

Immediately try to reconnect to relay first time
2 participants