fix: remove breaks in relay loop that could cause permanent disconnection from relay #869
Conversation
rolznz
changed the title
| @@ -123,7 +120,6 @@ func (svc *service) startNostr(ctx context.Context) error { | |||
| logger.Logger.WithError(err).Error("Got an error from the relay while listening to subscription.") | |||
| continue | |||
| } | |||
| break | |||
| } | |||
| select { | |||
| case <-ctx.Done(): | |||
There was a problem hiding this comment.
I think we can add a break here (after the log) instead of making it loop through once again and then break at L62
There was a problem hiding this comment.
I would like to keep it easy to understand by only having one exit point. The more places we can have breaks the more chance one of them doesn't work and incorrectly exits the loop.
| @@ -123,7 +120,6 @@ func (svc *service) startNostr(ctx context.Context) error { | |||
| logger.Logger.WithError(err).Error("Got an error from the relay while listening to subscription.") | |||
| continue | |||
| } | |||
| break | |||
There was a problem hiding this comment.
I think this break is here because it will be only hit when err == nil (which we assumed that it happens when ctx is done). And for that to happen, StartSubscription should return nil err, which can only happen when subscription context ends and
- NOT because of relay connection error
- NOT because of main context being cancelled (because otherwise breaking should be valid as the app is anyways terminated)
Is it possible? How to reproduce this?
There was a problem hiding this comment.
At first I think it would be good to log and see it happens. If we can find a way to reproduce it, then we can submit an issue or fix in go-nostr.
There was a problem hiding this comment.
I tried to reproduce this, and besides invalid filters that the relay rejects and closes the subscription but not the socket (which should not happen when using our properly configured relay) the only possible thing that might be happening is some sort of network socket read/write timeouts which causes the subscription to be closed but no connection error on the relay level. I did not reproduce such situation, so this is not 100% confirmed.
This would previously be treated as a program exit and so the break would cancel the loop.
+1 for removing them and making the reconnect loop more reliable.
|
@frnandu thanks. It definitely happens related to networking issue, more likely in self-hosted hubs. |
Fixes #888