Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Stop running queries on shutdown #95

Merged

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Apr 3, 2019

Make sure that any running queries are stopped when the DHT stops

@ghost ghost assigned dirkmc Apr 3, 2019
@ghost ghost added the status/in-progress In progress label Apr 3, 2019
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

The QueryManager should probably have a start method that gets called when the dht starts. While the use case may be low, it should be possible to start, stop and start the dht. Currently if that happen queries will fail to happen.

libp2p currently supports start/stop/start behavior. Sleeping/waking would be a potential case for this.

@dirkmc
Copy link
Contributor Author

dirkmc commented Apr 3, 2019

Good catch 👍
I pushed a fix

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This looks good. It might be worth it to add the test for attempting to run the query after stop, just to help prevent future regression issues with the code.

@dirkmc
Copy link
Contributor Author

dirkmc commented Apr 3, 2019

Makes sense - I added test cases for

  • passing an empty list of peers to Query.run()
  • calling Query.run() after shutdown

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @dirkmc

@vasco-santos vasco-santos merged commit e137297 into libp2p:master Apr 4, 2019
@ghost ghost removed the status/in-progress In progress label Apr 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants