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.

Sign up for GitHub

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

Jump to bottom

Unblock queues on shutdown or errors #10

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

Unblock queues on shutdown or errors #10

wants to merge 14 commits into from

Conversation

deavmi
Copy link
Owner

@deavmi deavmi commented Nov 26, 2023 •
edited
Loading

Purpose ✍️

We want to unblock all calls to dequeue() in two scenarios:

  1. If the underlying Stream (river stream) has an error (remote host disconnects)
  2. If the stop() method is called in the Manager

Todo ✅

  • On stop()
    • Implementation
    • Unittest
  • On Stream read failure (would be BClient actually)
    • Implementation
    • Unittest

Problems

  • User-initiated stop() can trigger WATCHER_FAILED as the BClient unblocks quicker to then call stop_FailedWatcher() triggering to save said exit reason, and then dequeue() wakes up before we race to set the correct exit reason for it to see instead of the old one
  • Shutdown default queue as well
  • Resources
    • After shutdown release all queues

@deavmi
Manager
569b029 
- Added a TODO for `stop()`
@deavmi deavmi mentioned this pull request Nov 26, 2023
Open
4 tasks
@deavmi deavmi self-assigned this Nov 26, 2023
@deavmi deavmi added bug Something isn't working enhancement New feature or request labels Nov 26, 2023
@deavmi
Watcher
6c9df71 
- Added TODO where we need to handling the unblocking of all `dequeue()` calls
@deavmi
ErrorType
198cb52 
- Added enum member `UNSET`
@deavmi
Queue
713c102 
- Added an `exitReason` and an `alive` (set to `true` on construction)
- Calling `shutdownQueue(ErrorType)` will set the exit reason, will also set the aliveness to `false` and wake up ALL `dequeue()`'s blocking
- `dequeue()` first check in wakeup routine duty cycle is to check if we are alive
@deavmi
ErrorType
08757f2 
- Added new member `MANAGER_SHUTDOWN`
@deavmi
Manager
31f7b63 
- Implemented `shutdownAllQueues()`
- Calling `stop()` now shuts down all queues
@deavmi
Copy link
Owner Author

deavmi commented Nov 26, 2023

Purpose ✍️

We want to unblock all calls to dequeue() in two scenarios:

  1. If the underlying Stream (river stream) has an error (remote host disconnects)
  2. If the stop() method is called in the Manager

Todo ✅

  • On stop()

    • Implementation
    • Unittest

  • On Stream read failure (would be BClient actually)

    • Implementation
    • Unittest

Working on this now, I should be able to finish this before the next weekend.

@deavmi
Copy link
Owner Author

deavmi commented Nov 26, 2023

I think I have the unblcoking of dequeue() when the manager is stop()'d working

@deavmi
Watcher
169f47d 
- Hoisted common imports into a `version(unittest)`

Watcher (unittests)

- Added a unittest for testing `stop()` to unblock `dequeue()`s
@deavmi
Watcher
7554703 
- Don't call `stop()` on the `Manager` when exiting the loop
@deavmi
ErrorType
aff9787 
- Added new member `WATCHER_FAILED`
@deavmi
Manager
c5a6612 
- Updated documentation for `stop()`
- Added `stop(ErrorType)` which will let you set the error type used for unblocking the `dequeue()` calls
- Added `stop_FailedWatcher()` which sets the `ErrorType` to `WATCHER_FAILED`
- `shutdownAllQueues()` now takes in a `ErrorType`
@deavmi
Watcher
8ac7a54 
- When we exit the read loop (upon an error) call `stop_FailedWatcher()` on the associated `Manager`

Watcher (unittests)

- Added a unit test for the above test case
@deavmi
Copy link
Owner Author

deavmi commented Nov 26, 2023

Also got this working for a failing remote end point now

@deavmi
Watcher (unitests)
815e411 
- Renamed
@deavmi
Watcher
93c7c43 
- Added a note about something we ought to fix
@deavmi
Manager
3bf9fd8 
- Added a TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deavmi deavmi

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@deavmi