Skip to content

Rework /internal/queue package#1449

Merged
dcantah merged 2 commits intomicrosoft:masterfrom
dcantah:mq-changes
Jul 13, 2022
Merged

Rework /internal/queue package#1449
dcantah merged 2 commits intomicrosoft:masterfrom
dcantah:mq-changes

Conversation

@dcantah
Copy link
Copy Markdown
Contributor

@dcantah dcantah commented Jul 7, 2022 •
edited
Loading

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
helsaawy
helsaawy reviewed Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
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

Comment thread internal/queue/mq.go Outdated
Comment thread internal/queue/mq.go Outdated
@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Jul 7, 2022 •
edited
Loading

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...

@dcantah
Rework /internal/queue package
3dfc47a 
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 <[email protected]>
@dcantah
Copy link
Copy Markdown
Contributor Author

dcantah commented Jul 7, 2022

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

anmaxvl
anmaxvl reviewed Jul 12, 2022
View reviewed changes
Comment thread internal/queue/mq.go
Comment thread internal/queue/mq.go
@dcantah
PR feedback
7f47808 
* Remove ErrQueueEmpty as it's not returned anywhere anymore.
* Add test for dequeue explicitly blocking when empty.

Signed-off-by: Daniel Canter <[email protected]>
@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
@dcantah
Rework /internal/queue package (microsoft#1449)
1154e47 
* 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 <[email protected]>
(cherry picked from commit 12d4cd8)
Signed-off-by: Daniel Canter <[email protected]>
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
@ambarve
Merged PR 6343733: Update ADO to f59b1f2
6a71173 
Related work items: #1437, #1447, #1449, #1453, #1455, #1457
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
@dcantah
Rework /internal/queue package (microsoft#1449)
a419987 
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anmaxvl anmaxvl anmaxvl approved these changes

@helsaawy helsaawy helsaawy approved these changes

Assignees

@anmaxvl anmaxvl

@helsaawy helsaawy

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dcantah @anmaxvl @helsaawy