-
Notifications
You must be signed in to change notification settings - Fork 168
Java: async getPubSubMessage.
#1770
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
Changes from all commits
a6f2b29
148eaf8
512eaa7
1586794
9b0cef3
e76d655
e0ca0a4
025de96
eb84c2a
b11e444
8ee4741
87e5739
72da45a
5729ca6
a483c1f
42d7907
a20febd
899adce
5fa4368
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */ | ||
| package glide.connectors.handlers; | ||
|
Yury-Fridlyand marked this conversation as resolved.
|
||
|
|
||
| import glide.api.models.PubSubMessage; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ConcurrentLinkedDeque; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| /** | ||
| * An asynchronous FIFO message queue for {@link PubSubMessage} backed by {@link | ||
| * ConcurrentLinkedDeque}. | ||
| */ | ||
| public class PubSubMessageQueue { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be package-private.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compilation fails -
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems strange that it uses it directly. I would've thought it goes through MessageHandler.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I see, you are creating a MessageHandler before the builder, then passing the handler in when creating the channel but passing the queue in to the client's constructor. This is exposing a class from a non-public package to a non-private field in a public/protected class in an exposed package. I am actually seeing that on a bunch of BaseClient classes. Let's discuss offline.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separate from the above issue of internal classes in public APIs, I think BaseClient should not have a MessageQueue. It should always route through a MessageHandler that accesses a MessageQueue. I would MessageQueue a static inner class of MessageHandler (package-private for unit testing). This would make it clear a MessageQueue can't really be on its own and is always tied to a MessageHandler. |
||
| /** The queue itself. */ | ||
| final ConcurrentLinkedDeque<PubSubMessage> messageQueue = new ConcurrentLinkedDeque<>(); | ||
|
|
||
| /** | ||
| * A promise for the first incoming message. Returned to a user, if message queried in async | ||
| * manner, but nothing received yet. | ||
| */ | ||
| CompletableFuture<PubSubMessage> firstMessagePromise = new CompletableFuture<>(); | ||
|
|
||
| /** A flag whether a user already got a {@link #firstMessagePromise}. */ | ||
| private final AtomicBoolean firstMessagePromiseRequested = new AtomicBoolean(false); | ||
|
|
||
| /** A private object used to synchronize {@link #push} and {@link #popAsync}. */ | ||
| private final Object lock = new Object(); | ||
|
|
||
| // TODO Rework to remove or reduce `synchronized` blocks. If remove it now, some messages get | ||
| // reordered. | ||
|
|
||
| /** Store a new message. */ | ||
| public void push(PubSubMessage message) { | ||
| synchronized (lock) { | ||
| if (firstMessagePromiseRequested.getAndSet(false)) { | ||
| firstMessagePromise.complete(message); | ||
| firstMessagePromise = new CompletableFuture<>(); | ||
| return; | ||
| } | ||
|
|
||
| messageQueue.addLast(message); | ||
| } | ||
| } | ||
|
|
||
| /** Get a promise for a next message. */ | ||
| public CompletableFuture<PubSubMessage> popAsync() { | ||
| synchronized (lock) { | ||
| PubSubMessage message = messageQueue.poll(); | ||
| if (message == null) { | ||
| // this makes first incoming message to be delivered into `firstMessagePromise` instead of | ||
| // `messageQueue` | ||
| firstMessagePromiseRequested.set(true); | ||
| return firstMessagePromise; | ||
| } | ||
| var future = new CompletableFuture<PubSubMessage>(); | ||
| future.complete(message); | ||
| return future; | ||
| } | ||
| } | ||
|
|
||
| /** Get a new message or null if nothing stored so far. */ | ||
| public PubSubMessage popSync() { | ||
| return messageQueue.poll(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.