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

Add Stack #4112

Open
wants to merge 8 commits into
base: series/3.x
Choose a base branch
from
Open

Add Stack #4112

wants to merge 8 commits into from

Conversation

BalmungSan
Copy link
Contributor

@BalmungSan BalmungSan commented Aug 3, 2024

  • Interface
  • Docs
  • Tests
  • Implementation

@reardonj
Copy link
Contributor

reardonj commented Aug 4, 2024

Queue, PQueue and Dequeue are split into Source/Sink interfaces to allow for Functor and Contravariant interfaces. This should probably have the same treatment?

@BalmungSan
Copy link
Contributor Author

Queue, PQueue and Dequeue are split into Source/Sink interfaces to allow for Functor and Contravariant interfaces. This should probably have the same treatment?

I thought about doing something like that, but TBH, not sure if it is really worth the effort.
IIRC, there is a general idea that the split of Queue didn't help that much, but I could be wrong.

Personally, I think that if you want to hide part of the interface and add extra functionality like map, that is better done in userland with a custom capability that encapsulates the Queue / Stack.

But, would love to hear what others think.

@BalmungSan BalmungSan changed the title WIP: Add Stack Add Stack Nov 25, 2024
@BalmungSan BalmungSan marked this pull request as ready for review November 25, 2024 23:19
// But, if we didn't managed to invalidate it.
// Then, that means we managed to receive a pushed element.
// Thus, we have to push it again to avoid it getting lost.
waiter.get.flatMap(element => this.push(element))
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this let elements slip under the top the stack if a push happens after the pop then before the push back? ie. stack of A,B,C:

  1. pop A but cancellation happens
  2. before push is called, another thread pushes D
  3. finalizer pushes A back

Now you'd have A,D,B,C, which should be impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, concurrent pop cancellation and push can change the order of elements in the Stack, I don't think there is much we can do here tho, we would need to store more information and the implementation would be very complex.

Also, note that for it to happen you actually need to wait, that can't happen on a non-empty Stack, only in an empty one.

So the real situation is something more akin to:
Stack is empty.
One fiber is waiting on pop
Another fiber pushes A, B, C, concurrently with pop is cancelled, and to another push(D)
The end result may be something like: C, D, B, A
Which, yes, feels wrong, but in a highly concurrent scenario, I'm not sure what else to do, at least the element didn't got lost and the overall priority is mostly preserved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is your scenario possible with pushN(A,B,C), or just a series of push()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible with pushN sadly, for series for pushs I don't mind since if they are all concurrent then the right order is undefined anyway AFAIK.
But for pushN I am indeed sad that it can lead to that error, the thing is that pushN is atomic in deciding which waiters to await, but it can't wait for them to be awakened before modifying the State.

@djspiewak djspiewak added this to the v3.7.0 milestone Dec 20, 2024
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.

3 participants