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

Dmsg delete entry on shutdown #883

Merged
merged 14 commits into from
Sep 16, 2021
Merged

Dmsg delete entry on shutdown #883

merged 14 commits into from
Sep 16, 2021

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented Sep 10, 2021

Did you run make format && make check?

Fixes #

Changes:

  • Added dmsgC.Close() in initDmsg in modules
  • Removed go dmsgC.Serve(ctx) from NewTransportListener as it was being called twice.

How to test this PR:

  1. Checkout dmsg repo on PR #110.
  2. In skywire uncomment replace github.com/skycoin/dmsg => ../dmsg in go.mod .
  3. Run make dep
  4. In skywire-services make log:"debug" in integration/generic/visorA.json
  5. Start integration env in skywire-services
  6. Run in different terminal
$ redis-cli
> get <PK-OF-VISORA>
  1. Wait for 5 mins and check again. It should still be there.
  2. Close visor A with Ctrl+C
  3. Check for the pk again in redis. The entry should be gone.

This can be tested without integration env on http://dmsg-discovery.myownsky.xyz.

pkg/visor/init.go Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor Author

ersonp commented Sep 13, 2021

@jdknives I was looking at the code and it seems like we need a nil check for TransportSetup module since transport_setup_nodes in the config is always null. And if I am not wrong then it only works when transport_setup_nodes has a list of pk's.

@jdknives
Copy link
Member

@jdknives I was looking at the code and it seems like we need a nil check for TransportSetup module since transport_setup_nodes in the config is always null. And if I am not wrong then it only works when transport_setup_nodes has a list of pk's.

We need to define a PubKey for that service before we push out the release so it is not null by default. If it is though by user intervention, we should not initialize the module.

Copy link
Contributor

@alexadhy alexadhy left a comment

Choose a reason for hiding this comment

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

just proper error checks thing for consistency

pkg/router/router.go Outdated Show resolved Hide resolved
pkg/transport/setup/visor.go Outdated Show resolved Hide resolved
pkg/visor/hypervisor.go Outdated Show resolved Hide resolved
@jdknives jdknives merged commit 9e363ef into skycoin:develop Sep 16, 2021
@ersonp ersonp deleted the dmsg-delentry branch April 11, 2022 15:38
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.

3 participants