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

PriorityMailbox #364

Merged
merged 14 commits into from
Sep 4, 2014
Merged

PriorityMailbox #364

merged 14 commits into from
Sep 4, 2014

Conversation

rogeralsing
Copy link
Contributor

  • Contains a new generic mailbox base Mailbox<TSys,TUser> which can consume different kinds of message queues
  • Contains UnboundedPriorityMailbox base class that can be used to create custom priority mailboxes
  • Contains MessageQueue and related semantics

Related to #299 and #301

@Aaronontheweb
Copy link
Member

@rogeralsing took a look at the PriorityQueue implementation, the mailbox changes, and the specs for the PriorityMailbox. My thoughts:

  1. The priority queue implementation looks good - put an XML-DOC comment at the top of the class describing which data structure it's using (i.e. BinaryHeap) for reference in case any other developers are interested or want to add some special case mailbox implementations in the future (there might be some data structures that can offer much better sorting performance if they're bounded by a maximum size, for instance.)
  2. The message queue semantics look good - I think that initialization process looks like it makes sense.
  3. For developers who want to use the PriorityMailbox, they need to subclass it and refer to that via a FQN in their Props just like in Akka, right?
  4. A couple of ideas on the priority queue specs: (1) I'd add an actor to the TestKit whose Receive method blocks (ManualResetEvent or something) until you explicitly allow it to run, built for the purpose of testing mailbox implementations. That will help eliminate some of the racy stuff and make it easy to test things like "what happens if this bounded mailbox reaches its maximum size?" (2) I'd do some tests for inserting a low-priority message behind a long queue of high priority messages (1000 messages with priority 1, add a message with priority 2) and vice-versa. Mostly just to verify that the correctness of the heap implementation (it's easier to expose problems with the balancing algorithm at larger sizes.)

Looks solid overall!

@rogeralsing
Copy link
Contributor Author

  1. Added a description and source reference for the priority queue

  2. Yes, mailboxes are named and has a FQN type associated.

  3. Race condition problems are fixed by suspending the mailbox and resuming it after all messages have been enqueued.

Also Improved the spec to send more messages of different priorities.

@Aaronontheweb
Copy link
Member

@rogeralsing ah, forgot you can just call suspend / resume on the mailbox directly.


//drain envelopes from ConcurrentQueue into priority queue
Envelope tmp;
while (_queue.TryDequeue(out tmp))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dequeue all messages from concurrent queue into prio queue

Copy link
Member

Choose a reason for hiding this comment

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

is there a possibility that under a heavy load this section of code could live-lock? i.e. never leave the while loop?

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 it could.
However, one would have to take into consideration that a normal unbounded mailbox will yield out of memmory exception and kill the entire system in the same scenario given enough time.
So unbounded queues are not safe for extreme load.
(thus the introduction of akka streams)

So I guess we have three ways to deal with this:

  1. We leave it as is and just accept that unbounded queues can be harmful.
  2. We make the loop exit after x times, this will make the priority of messages less exact,
    but it can never be more than best effort anyway.
    e.g. send a low prio message and wait one hour and send a high prio message, the low prio message will ofcourse arrive first no matter what approach we take.
  3. We make it blocking. (which still only gives best effort, se comment above)

I'm open to any of the approaches

Copy link
Member

Choose a reason for hiding this comment

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

I would make the priority queue implementation thread-safe and have it block and write directly to that - we'll be taking a bit of a perf hit since it won't have the native optimizations that ConcurrentQueue has but live-locking this code might not be that hard to do (a for...loop might be enough.) I'd err on the side of predictability in this case.

Introduced a blocking wrapper base class for message queues that use non thead safe data structures
/// <summary>
/// Base class message queue that uses a priority generator for messages
/// </summary>
public class UnboundedPriorityMessageQueue : BlockingMessageQueue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The priority queue is now used in a blocking message queue base class.

@Aaronontheweb
Copy link
Member

👍 cool, looks good - let me get #351 out today and then we'll merge this in and include it as part of a v0.7 release.

@Aaronontheweb Aaronontheweb mentioned this pull request Sep 3, 2014
@Aaronontheweb
Copy link
Member

It's safe to pull this into the code base now. All of the Akka v0.6.4 stuff is out and has an open PR going into master from my personal master branch, so any changes you make to dev at this point are fine.

rogeralsing added a commit that referenced this pull request Sep 4, 2014
@rogeralsing rogeralsing merged commit afe37d8 into akkadotnet:dev Sep 4, 2014
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.

2 participants