Skip to content

Fire SecureChannelOpenedEvent only after successful write of OpenSecureChannelResponse#1717

Merged
kevinherron merged 2 commits intofeature/reverse-connectfrom
copilot/sub-pr-1716
Mar 25, 2026
Merged

Fire SecureChannelOpenedEvent only after successful write of OpenSecureChannelResponse#1717
kevinherron merged 2 commits intofeature/reverse-connectfrom
copilot/sub-pr-1716

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 25, 2026

SecureChannelOpenedEvent was fired immediately after writeAndFlush(..., voidPromise()), which provides no delivery guarantee — allowing the reverse-connect FSM to transition to Active before the response was actually sent.

Changes

  • UascServerAsymmetricHandler: Replace voidPromise() write with a captured ChannelFuture; fire SecureChannelOpenedEvent in the success listener only after the write completes successfully
  • Write failure handling: On failure, log the error and close the channel
// Before
ctx.writeAndFlush(chunkComposite, ctx.voidPromise());
ctx.pipeline().fireUserEventTriggered(new SecureChannelOpenedEvent(secureChannel.getChannelId()));

// After
ChannelFuture writeFuture = ctx.writeAndFlush(chunkComposite);
writeFuture.addListener(future -> {
    if (future.isSuccess()) {
        ctx.pipeline().fireUserEventTriggered(new SecureChannelOpenedEvent(secureChannel.getChannelId()));
        logger.debug("Sent OpenSecureChannelResponse.");
    } else {
        logger.error("Error writing OpenSecureChannelResponse: {}", future.cause().getMessage(), future.cause());
        ctx.close();
    }
});

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • repo.eclipse.org
    • Triggering command: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.18-8/x64/bin/java /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.18-8/x64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.13/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.13/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.13 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.13/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/milo/milo org.codehaus.plexus.classworlds.launcher.Launcher -q spotless:apply -pl opc-ua-stack/transport (dns block)
    • Triggering command: /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.18-8/x64/bin/java /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.18-8/x64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.13/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.13/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.13 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.13/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/milo/milo org.codehaus.plexus.classworlds.launcher.Launcher -q clean compile -pl opc-ua-stack/transport --also-make (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] [WIP] Address feedback on reverse connect support implementation Fire SecureChannelOpenedEvent only after successful write of OpenSecureChannelResponse Mar 25, 2026
Copilot AI requested a review from kevinherron March 25, 2026 13:20
@kevinherron kevinherron marked this pull request as ready for review March 25, 2026 13:21
@kevinherron kevinherron merged commit 3dda5de into feature/reverse-connect Mar 25, 2026
1 check passed
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