Generalized notif stream #826
Conversation
sputn1ck
force-pushed
the
generalized_notif_stream
branch
3 times, most recently
from
e8ddc78
to
0bc48d0
Compare
sputn1ck
force-pushed
the
generalized_notif_stream
branch
from
0bc48d0
to
506e218
Compare
| select { | ||
| case <-sub.subCtx.Done(): | ||
| close(recvChan) | ||
| continue |
There was a problem hiding this comment.
Should it be break here? Otherwise we can close recvChan multiple times (panic).
There was a problem hiding this comment.
I think it's correct with continue. My linter highlights the for loop if I have this continue selected 😆
There was a problem hiding this comment.
You are right! I confused sub.subCtx with ctx :)
|
|
||
| select { | ||
| case <-time.After(10 * time.Second): | ||
| return fmt.Errorf("notification manager not ready") |
There was a problem hiding this comment.
Is 10 seconds sufficient? If L402 fails and retries, fetchL402 already waits for 10 seconds, so it will crash here by the time of the second attempt. Also in Tor 10 seconds is not enough.
notifications/manager.go
Outdated
|
|
||
| cancel() | ||
| // Wait for a bit before reconnecting. | ||
| <-time.After(time.Second * 10) |
There was a problem hiding this comment.
I propose this:
select {
case <-time.After(time.Second * 10):
case <-ctx.Done():
}not to keep this goroutine running for 10 seconds after main context is cancelled.
There was a problem hiding this comment.
Have a new timer version up in a bit, which waits 0 seconds on the first try!
| return nil | ||
| default: | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: I propose to move this code into a new method. Then we can defer cancel() in that method.
notifications/manager.go
Outdated
| return notifChan | ||
| } | ||
|
|
||
| // Run starts the notification manager. |
There was a problem hiding this comment.
I propose to add to godoc, that it closes readyChan as soon as it makes first successful connection to the stream. Also worth noting, that the function is long-running (not launching a goroutine and returning soon).
sputn1ck
force-pushed
the
generalized_notif_stream
branch
2 times, most recently
from
a4152d1
to
d4a3f29
Compare
This commit refactors the proto definitions of the reservation notifications to use a more generic mechanism that can be used for other types of notifications as well.
This commit removes the notification stream from the reservation manager and replaces it with a subscriber interface.
sputn1ck
force-pushed
the
generalized_notif_stream
branch
2 times, most recently
from
666edcd
to
882c010
Compare
This commit adds a generic notification manager that can be used to subscribe to different types of notifications.
This commit adds the notification manager to the loopd daemon.
sputn1ck
force-pushed
the
generalized_notif_stream
branch
from
882c010
to
1ce867f
Compare
This PR adds a generalized notif stream that replaces specific streams such as the reservations listener.