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

fix: remove _running/stop machinery #99

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

tomblench
Copy link
Contributor

  • while (!stop.get()) logic was faulty since the loop will always be exited with 0 changes, some changes, or null.
  • _running and stop variables not needed: if connector is removed or shutdown using the connect REST api, then stop will be called on the same thread as poll. Therefore these methods can be considered fully synchronous and there is no such thing as an "in flight" or "pending" request to cloudant. This was observed in both standalone and distributed configuration.
  • Narrow scope of cloudantChangesResult by moving to local variable.

Fixes #94

- `while (!stop.get())` logic was faulty since the loop will always be
  exited with 0 changes, some changes, or null.
- `_running` and `stop` variables not needed: if connector is removed
  or shutdown using the connect REST api, then `stop` will be called
  on the same thread as `poll`. Therefore these methods can be
  considered fully synchronous and there is no such thing as an "in
  flight" or "pending" request to cloudant. This was observed in both
  standalone and distributed configuration.
- Narrow scope of `cloudantChangesResult` by moving to local variable.
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

I think we should also remove the associated (unimplmented test) at

@ricellis
Copy link
Member

oh sorry nvm, that's a different stop(!)

@tomblench tomblench merged commit 30dee4b into main Sep 20, 2022
@tomblench tomblench deleted the 94-sync-stop-cancel-inflight-requests branch September 20, 2022 13:13
@ricellis ricellis added this to the 0.200.0 milestone Sep 20, 2022
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.

Synchronise stop method and cancel in-flight requests
2 participants