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

lnc: Add a timeout when creating the grpc client with the mailbox #127

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

positiveblue
Copy link
Contributor

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

@positiveblue positiveblue changed the title loc: Add a timeout when creating the grpc client with the mailbox lnc: Add a timeout when creating the grpc client with the mailbox Feb 10, 2024
Copy link
Member

@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 fix! Code looks good, just a question about context lifetime.

lnc/lnc.go Outdated Show resolved Hide resolved
ctxt, cancelT := context.WithTimeout(
context.Background(), DefaultConnectionTimetout,
)
defer cancelT()
Copy link
Member

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?

Copy link
Contributor Author

@positiveblue positiveblue Feb 12, 2024

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.
@guggero
Copy link
Member

guggero commented Feb 13, 2024

@positiveblue the code looks good, thanks. Maybe we should also bump the LNC dependency to the recently released version v0.3.0 that fixes a couple of issues. That probably will also help increasing the connection stability and reliability.

The lates version of lightning-node-connect (`v0.3.0-alpha`)
requires at least that version.
@positiveblue
Copy link
Contributor Author

lightning-node-connect version bumped 👍

I also had to bump the minim go version for compiling the project because LNC needs go 1.21

Copy link
Member

@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.

utACK, LGTM 🎉

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.

No response when dialing to Mailbox via gPRC
2 participants