Skip to content

feat: add MUX method that waits for a connection.#708

Draft
alcarraz wants to merge 7 commits into
jpos:mainfrom
alcarraz:feature/mux-wait-for-connection
Draft

feat: add MUX method that waits for a connection.#708
alcarraz wants to merge 7 commits into
jpos:mainfrom
alcarraz:feature/mux-wait-for-connection

Conversation

@alcarraz

@alcarraz alcarraz commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

This PR is more of an issue in disguise, and that is why it is maked as Draft. I've had this idea for some weeks now, and I figured it would be easy to propose a POC instead of describing what the added methods would do.

When we need to wait for a MUX to connect during some timeout, we usually end up doing some kind of polling, like in QueryHost. But since the muxes detect the connection using a Space ready key, it would be better to just read the entry with a timeout.

Of course, the developer could just do the read, but that would involve the developer knowing the key and type of mux it is using, leaving the code highly coupled.

This POC adds an isConnected(long timeout that blocks until the timeout expires or the mux is connected. Perhaps the method should be called differently, like waitForConnection, which expresses its meaning more clearly. I don't have a clear favourite.

In the interface, we provide a default implementation that performs the usual polling. So it does not break other mux implementations. This should be both runtime and compile-time backward compatible.

I just realized that the method could be added to the ISOSource interface instead, but I have mixed feelings about that. On one side, it doesn't cost anything, and it could cover more cases. On the flip side, it makes less sense semantically since a standard source would be connected and then disconnected, and not the other way around. However, we could think of some sources that are intermittently connected, e.g., strange setups where a "reverse" mux acting as a server can accept responses through a different connection than the request. Or some bidirectional communication implemented as a pair of "queues".

In this POC, I also rewrote the QueryHost participant to showcase how this would look.

If this feature makes sense, I can try to improve this PR with some more time of dedication.

This POC also does not consider timeout of zero or negative a special case, i.e., they will return immediately as isConnected() would, but we could consider some special case for indefinitely waiting, or just throw an exception on a negative value. I lightly prefer the current option because it's better for corner cases in the calling logic where the timeout just happens to compute to zero or negative, in which case the expected result would be to return immediately.

This implementation could look cleaner with structured concurrency.

Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
@ar

ar commented Apr 22, 2026

Copy link
Copy Markdown
Member

I like it. Great idea. isConnected(timeout) is quite intuitive, so let's use it.

Perhaps the argument that the `ISOSource` wouldn't connect after disconnection is not strong enough to limit the possibility of having it here. For instance, a MUX connected to a client channel adaptor that can receive responses in a different connection. Not a frequent scenario, but a probable one.

Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
Consider the possibility that if the mux is not enabled at first, can be enabled during the timeout window

Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
@ar-agt

ar-agt commented May 21, 2026

Copy link
Copy Markdown
Collaborator

I reviewed this locally against the PR base (c2e712d) and ran the focused tests on the PR branch.

Findings:

  1. git diff --check fails.

    There are trailing whitespace/newline issues in:

    • jpos/src/main/java/org/jpos/iso/ISOSource.java:46
    • jpos/src/main/java/org/jpos/q2/iso/MUXPool.java:350,358,359
    • jpos/src/test/java/org/jpos/q2/iso/MUXPoolTest.java:157,268
    • jpos/src/test/java/org/jpos/q2/iso/QMUXTest.java:435 — new blank line at EOF
  2. MUXPool.isConnected(timeout) looks more complex than it needs to be.

    The implementation starts one virtual-thread task per mux, then for checkEnabled QMUXes submits two more tasks to the same executor and waits on inner futures. The current focused tests pass, but this is hard to reason about in terms of timeout ownership, cancellation, and simultaneous connected/enabled state. Since MUXPool already has simple polling/deadline behavior elsewhere, I’d prefer a simpler deadline loop that checks connected-with-remaining-time plus enabled/ready state without nested futures.

  3. QMUX.isConnected(timeout) returns running() immediately when no ready indicators are configured.

    This preserves existing isConnected() semantics, but for a new “wait for connection” API it means a running QMUX without <ready> never waits for anything more concrete. That may be intended, but it should be explicit in Javadoc/tests because callers like QueryHost now depend on this method for reachability waiting.

Verification:

./gradlew :jpos:test --tests org.jpos.q2.iso.QMUXTest --tests org.jpos.q2.iso.MUXPoolTest

Result: BUILD SUCCESSFUL on the PR branch.

git diff --check c2e712d...pr-708

Result: failed with the whitespace issues listed above.

Overall: useful feature direction and the focused tests pass, but I’d fix the whitespace before merge and consider simplifying MUXPool.isConnected(timeout) while this is still draft.

…w feedback

Replace the nested Future-based wait in MUXPool.isUsable(mux, timeout, executor)
with CompletableFuture.allOf, so the connected and enabled/ready checks share a
single timeout budget instead of the second get() silently getting less time
than the first left it.

Also documents QMUX.isConnected(timeout)'s fallback to running() when no
<ready> indicators are configured, with a covering test, and fixes the
trailing-whitespace / stray-blank-line issues flagged by review.

Signed-off-by: Andrés Alcarraz <andres.alcarraz@transactility.com>
@alcarraz

alcarraz commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Re: point 2 (MUXPool.isConnected(timeout) complexity / nested futures)

I considered a plain deadline-poll loop instead (mirroring ISOSource's default isConnected(timeout), using the existing non-blocking isUsable(MUX) check on a sleep interval). I decided against it: the idea of this PR is to replace hand-rolled polling (like the old QueryHost) with a real blocking wait on the space, so reintroducing a sleep-poll loop internally, even hidden behind the public API, feels like going in the opposite direction.

What I did fix is the actual bug behind the complexity: previously the two blocking waits (mux.isConnected(wait) and sp.rd(enabledKey, wait)) were .get()'d sequentially, so the second one didn't share the first's remaining timeout budget. They're now composed with CompletableFuture.allOf(conn, enab).get(wait, ...), so a single timeout genuinely bounds both concurrently.

The nested-future shape itself is what I'd expect real concurrent composition of two independent blocking waits to look like without a language-level structured-concurrency primitive. StructuredTaskScope would clean this up (and is what I'd reach for), but it's still in preview state.

Whitespace and the QMUX no-<ready> documentation/test are fixed in the latest commits.

If you still feel strongly about eliminating the nesting, I'm OK with implementing the polling version for now, and revisit this with structured concurrency once it exits the preview state.

@alcarraz
alcarraz marked this pull request as ready for review July 8, 2026 23:22
@alcarraz
alcarraz marked this pull request as draft July 8, 2026 23:27
@ar

ar commented Jul 9, 2026

Copy link
Copy Markdown
Member

Given this uses virtual threads and the two waits now share a single timeout budget, I’m OK keeping the blocking Space-based implementation rather than switching back to polling. I'm re-running actions that were failing to see if it was a transient issue.

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