-
Notifications
You must be signed in to change notification settings - Fork 59
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
lnc: Add a timeout when creating the grpc client with the mailbox #127
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Code looks good, just a question about context lifetime.
ctxt, cancelT := context.WithTimeout( | ||
context.Background(), DefaultConnectionTimetout, | ||
) | ||
defer cancelT() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we can cancel this context here? Is the context only used for the initial dialing and not for the whole duration of the connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I had to check it before opening the PR because I wasn't sure neither.
Form the grpc docs:
In the blocking case, ctx can be used to cancel or expire the pending connection. Once this function returns, the cancellation and expiration of ctx will be noop. Users should call ClientConn.Close to terminate all the pending operations after this function returns.
Make sure that we do not hold forever if we are not able to dial with the maiblox service. This could happen in cases where a passphrase is being reused and the secret key has already been negotiated for example.
ec4e929
to
5662500
Compare
@positiveblue the code looks good, thanks. Maybe we should also bump the LNC dependency to the recently released version |
The lates version of lightning-node-connect (`v0.3.0-alpha`) requires at least that version.
I also had to bump the minim go version for compiling the project because LNC needs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, LGTM 🎉
Do not hang forever when creating the grpc client with the mailbox.
Also, make sure we use the
DefaultStore
timeouts every time we call store functions in the package.Both timeouts are currently set to 10s.
Fix #124