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

Rework /internal/queue package #1449

Merged
merged 2 commits into from
Jul 13, 2022
Merged

Rework /internal/queue package #1449

merged 2 commits into from
Jul 13, 2022

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jul 7, 2022

Given our use cases for this package, we don't need methods that don't block
on reads if there's no value to be read. Due to this, I've removed the
ReadOrWait function and did a small redesign of the methods to be more
in line with standard queue method naming.

  • Change Read/Write/IsEmpty to Dequeue/Enqueue/Size and remove ReadOrWait.
    Now there is no version of Read/Dequeue that doesn't block if the queue
    is empty.
  • Fix up tests to be in line with this removal of the non-blocking read
    and simplified most of the tests.

@dcantah dcantah marked this pull request as ready for review July 7, 2022 00:24
@dcantah dcantah requested a review from a team as a code owner July 7, 2022 00:24
@helsaawy helsaawy self-assigned this Jul 7, 2022
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

A couple of thoughts:

  1. Would a DequeueNoWait() be useful at all?
  2. I kind of like the RWMutex, for a couple reasons:
    a. if we add a Peek() (any, error), then it a RLock would make sense
    b. if we use the RLock at the beginning of De/Enqueue to check if the queue is closed/empty, then multiple attempts to read/write can fail simultaneously instead of each waiting in turn, so a moderate optimization (though I doubt performance is currently a concern)
  3. since MessageQueue shouldnt be copied (cause of closed bool), there a gross hack that sync uses to raise go vet errors that we may want to use here

@dcantah
Copy link
Contributor Author

dcantah commented Jul 7, 2022

A couple of thoughts:

  1. Would a DequeueNoWait() be useful at all?
  2. I kind of like the RWMutex, for a couple reasons:
    a. if we add a Peek() (any, error), then it a RLock would make sense
    b. if we use the RLock at the beginning of De/Enqueue to check if the queue is closed/empty, then multiple attempts to read/write can fail simultaneously instead of each waiting in turn, so a moderate optimization (though I doubt performance is currently a concern)
  3. since MessageQueue shouldnt be copied (cause of closed bool), there a gross hack that sync uses to raise go vet errors that we may want to use here
  1. I removed it as for our purposes at the moment we'd always want to block, but if we find future use cases where it may come in handy we could add it back in
  2. We couldn't read lock for Dequeue right? We could up until the close check but after we'd just need to lock for the actual modify on the queue. Although agree if we could get away with rlock it'd be nice. rlock for peek makes sense, but I'm not sure we have a use for it here; it would also help for Size(), but we don't even use Size() in this codebase 🤣
  3. Oh my word...

Given our use cases for this package, we don't need methods that don't block
on reads if there's no value to be read. Due to this, I've removed the
ReadOrWait function and did a small redesign of the methods to be more
in line with standard queue method naming.

* Change Read/Write/IsEmpty to Dequeue/Enqueue/Size and remove ReadOrWait.
Now there is no version of Read/Dequeue that doesn't block if the queue
is empty.
* Fix up tests to be in line with this removal of the non-blocking read
and simplified most of the tests.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah
Copy link
Contributor Author

dcantah commented Jul 7, 2022

@helsaawy I just stuck with rwmutex as we can use it for Size check at least

* Remove ErrQueueEmpty as it's not returned anywhere anymore.
* Add test for dequeue explicitly blocking when empty.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah merged commit 12d4cd8 into microsoft:master Jul 13, 2022
dcantah added a commit to dcantah/hcsshim that referenced this pull request Jul 21, 2022
* Rework /internal/queue package

Given our use cases for this package, we don't need methods that don't block
on reads if there's no value to be read. Due to this, I've removed the
ReadOrWait function and did a small redesign of the methods to be more
in line with standard queue method naming.

* Change Read/Write/IsEmpty to Dequeue/Enqueue/Size and remove ReadOrWait.
Now there is no version of Read/Dequeue that doesn't block if the queue
is empty.
* Fix up tests to be in line with this removal of the non-blocking read
and simplified most of the tests.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
(cherry picked from commit 12d4cd8)
Signed-off-by: Daniel Canter <dcanter@microsoft.com>
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
* Rework /internal/queue package

Given our use cases for this package, we don't need methods that don't block
on reads if there's no value to be read. Due to this, I've removed the
ReadOrWait function and did a small redesign of the methods to be more
in line with standard queue method naming.

* Change Read/Write/IsEmpty to Dequeue/Enqueue/Size and remove ReadOrWait.
Now there is no version of Read/Dequeue that doesn't block if the queue
is empty.
* Fix up tests to be in line with this removal of the non-blocking read
and simplified most of the tests.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
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.

None yet

3 participants