Skip to content

Binary eio v3 + Engine IO v3, v4 Interop JS integration tests - #228

Open
sanjomo wants to merge 68 commits into
mainfrom
binary-eio-v3
Open

Binary eio v3 + Engine IO v3, v4 Interop JS integration tests#228
sanjomo wants to merge 68 commits into
mainfrom
binary-eio-v3

Conversation

@sanjomo

@sanjomo sanjomo commented Jul 30, 2026

Copy link
Copy Markdown
Member

Description

Brief description of the changes in this PR.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Test improvements
  • Build/tooling changes

Related Issue

Closes #(issue number)

Changes Made

Testing

  • All existing tests pass
  • New tests added for new functionality
  • Tests pass locally with mvn test
  • Integration tests pass (if applicable)

Checklist

  • Code follows project coding standards
  • Self-review completed
  • Code is commented where necessary
  • Documentation updated (if needed)
  • Commit messages follow conventional format
  • No merge conflicts
  • All CI checks pass

Additional Notes

Any additional information, screenshots, or context that reviewers should know.


Open in Devin Review

Summary by CodeRabbit

  • New Features
    • Added APIs for removing connection and disconnection listeners.
    • Added binary-safe event serialization.
    • Added a clustered example with distributed rooms, broadcasts, acknowledgments, and typed payloads.
  • Bug Fixes
    • Improved Engine.IO v2–v4 polling and WebSocket compatibility, including binary attachments and response content types.
    • Improved per-connection packet handling and distributed event reliability.
    • Improved graceful shutdown handling for Redis and Kafka event stores.
  • Tests
    • Expanded JavaScript interoperability, distributed scenarios, protocol compatibility, and resource-leak coverage.

sanjomo added 3 commits July 30, 2026 20:19
Add correct Engine.IO version handling and robust binary framing support across transports. Introduce Packet.withEngineIOVersion to avoid mutating shared Packet instances during broadcasts; update Namespace, SingleRoomBroadcastOperations, EncoderHandler, PacketEncoder and PacketDecoder to emit/parse EIOv2/v3 polling wrappers, EIOv3 'b4' base64 text, WebSocket prefixes (0x04) and EIOv4 text/plain behavior. Enhance packet decoding with transport-aware logic and multi-packet separator handling. Enable safe polymorphic Jackson typing in Kafka/Hazelcast/NATS serializers/deserializers. Add many integration tests and JS interop fixtures (js-interop resources, test clients), update test containers/configs (Hazelcast image/xsd, Kafka consumer group/offsets), improve logging and example to show Hazelcast-backed clustering. Update .gitignore and bump dependencies in examples/pom.xml.
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds Engine.IO v2–v4 transport-aware packet processing, per-client packet isolation, shared event serialization, JavaScript and distributed integration tests, CI Node.js setup, Hazelcast updates, and a clustered Hazelcast-backed example.

Changes

Protocol and serialization

Layer / File(s) Summary
Engine.IO codec and packet isolation
netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/*, .../handler/*, .../namespace/*
Packet handling preserves per-client Engine.IO versions and supports transport-specific v2/v3/v4 framing, binary wrappers, polling separators, and attachment decoding.
Distributed event serialization and lifecycle
netty-socketio-core/src/main/java/com/socketio4j/socketio/store/*, .../SocketIOServer.java, .../listener/*, .../namespace/*
A shared Jackson mapper serializes and restores byte[] values across event stores. Listener removal APIs, null-event handling, and Redisson shutdown handling are updated.

Interoperability and tests

Layer / File(s) Summary
JavaScript client interoperability
netty-socketio-core/src/test/resources/js-interop/*, .../integration/JsClientInteropTest.java, .../integration/ProtocolScenariosIntegrationTest.java, .../integration/EIOv3*
Node clients and Java tests cover Socket.IO versions 1–4, transports, acknowledgements, binary payloads, objects, POJOs, and Engine.IO v3 scenarios.
Distributed integration coverage
netty-socketio-core/src/test/java/com/socketio4j/socketio/integration/Distributed*
Distributed tests add deterministic room synchronization, EIO v3 forwarding, JavaScript client matrices, and Hazelcast, Kafka, NATS, Redis Streams, and Redis PubSub backends.
Protocol robustness and resource tests
netty-socketio-core/src/test/java/com/socketio4j/socketio/{protocol,handler,transport,leak,namespace}/*
Tests cover malformed packets, buffer leaks, concurrent room joins, invalid URLs, HTTP headers, WebSocket frames, server restarts, and memory-store cleanup.

Build and example

Layer / File(s) Summary
CI and clustered example
.github/workflows/build.yml, .gitignore, pom.xml, netty-socketio-examples/netty-socketio-core-example/*, netty-socketio-core/src/test/resources/hazelcast-test-config.xml
CI installs Node.js interop dependencies, Hazelcast versions and module opens are updated, and the example runs two Hazelcast-backed Socket.IO servers.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: neatguycoding

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description retains the template but provides no concrete summary, selected change type, testing results, checklist status, or additional context. Complete the description with the actual changes, related issue, selected change type, test results, checklist status, and relevant reviewer notes.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main EIO v3 protocol and JavaScript interoperability test changes.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch binary-eio-v3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

github-code-quality[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

sanjomo added 2 commits July 31, 2026 00:33
Add missing listener removals (ClientListeners/Namespace/SocketIOServer). Harden EncoderHandler/PacketDecoder parsing for Engine.IO polling: skip leading 0x1E delimiters, validate legacy length-prefixed polling wrappers, guard against unknown EngineIOVersion and use safe equals checks. Change PacketEncoder to emit 0x1E only for EIOv4 and use standard Base64 for EIOv4 polling attachments so browser clients decode '+' and '/'. Tweak EventMessageJsonSupport ObjectMapper to avoid empty-bean failures and related types. Add and update integration/unit tests and JS test client scenarios. Update test hazelcast container image and example pom dependency. Several test cleanup/logging robustness fixes.
Adjust polling encoding and HTTP headers to correctly support Engine.IO v4. EncoderHandler now selects text/plain for v4 polling (and for non-binary payloads), and PacketEncoder was rewritten to handle EIO v4 multi-packet polling (0x1E delimiter) with STANDARD Base64 for attachments, while preserving v2/v3 binary envelope behavior. Buffer handling, length-prefixing and resource releases were improved and unsupported versions now throw. Unit tests updated to reflect v4/v3 differences, JSONP behavior, and use StandardCharsets.
devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

sanjomo added 2 commits July 31, 2026 14:33
PacketDecoder: treat PONG like PING (read text payload), fix polling length-header digit validation, and add explicit ERROR body parsing with JSON fallback. Tests: significantly expand PacketDecoderTest and PacketEncoderTest to cover Engine.IO V2/V3/V4, binary attachments, XHR2 polling binary frames, ping/pong, ACKs and ERRORs. Integration tests and JS interop fixtures updated to improve distributed room/isolation/leave checks and to fail/pass cleanly; added connect/event presence assertions in JsClientInteropTest. Minor test whitespace cleanup. These changes improve protocol compatibility and increase test coverage for binary and cross-version behaviors.
Suppress XREAD error noise during shutdown in RedisStreamEventStore by checking the running flag and detecting Redisson shutdown (exception type/cause/message). Add a suite of robustness and edge-case unit tests: WrongUrlHandlerTest, ByteBufLeakTest (Netty PARANOID leak detection), PacketDecoderFuzzingTest, SocketSslServerRestartTest (rapid restarts), WebSocketTransportTest (binary frame + mocks), HttpTransportTest (polling headers), MemoryStoreTest (session expunge), and NamespaceTest (concurrent room joins). Also adjust test helpers/mocks where needed.
github-code-quality[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Remove an unused JsonTypeInfo import from Packet.java. Rework PacketDecoder branching for polling payloads: move the Base64 polling ('b') handling (and its separator-slicing logic) into the else-if branch and place the fallback binary polling payload in the final else branch. This clarifies control flow and ensures the polling attachment paths are handled correctly.
devin-ai-integration[bot]

This comment was marked as resolved.

Use try-with-resources and finally to properly release ByteBuf/InputStream resources in PacketDecoder and PacketEncoder to prevent leaks and correctly handle JSONP framing. Add @OverRide annotations to PacketDecoderFuzzingTest lifecycle methods. Add Apache-2.0 license headers to JS test resource files. These changes improve resource safety and clarity without altering protocol behavior.
devin-ai-integration[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 2 unresolved review comments.

Files modified:

  • netty-socketio-core/src/test/java/com/socketio4j/socketio/SocketSslServerRestartTest.java
  • netty-socketio-core/src/test/java/com/socketio4j/socketio/protocol/PacketDecoderFuzzingTest.java

Commit: 73254cb437855750a8864fd185413fef0a55f405

The changes have been pushed to the binary-eio-v3 branch.

Time taken: 2m 32s

Fixed 2 file(s) based on 2 unresolved review comments.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
devin-ai-integration[bot]

This comment was marked as resolved.

server always gets immutable copy of config so changing port have no effect
devin-ai-integration[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #229

coderabbitai Bot added a commit that referenced this pull request Aug 1, 2026
Docstrings generation was requested by @sanjomo.

* #228 (comment)

The following files were modified:

* `netty-socketio-core/src/main/java/com/socketio4j/socketio/SingleRoomBroadcastOperations.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOServer.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/EncoderHandler.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/handler/InPacketHandler.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/listener/ClientListeners.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/namespace/Namespace.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/JacksonJsonSupport.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/Packet.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketDecoder.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/protocol/PacketEncoder.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/event/EventMessageJsonSupport.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/hazelcast/HazelcastPubSubEventStore.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/kafka/serialization/EventMessageDeserializer.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/kafka/serialization/EventMessageSerializer.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/nats_pubsub/EventMessageCodec.java`
* `netty-socketio-core/src/main/java/com/socketio4j/socketio/store/redis_stream/RedisStreamEventStore.java`
* `netty-socketio-core/src/test/java/com/socketio4j/socketio/store/CustomizedHazelcastContainer.java`
* `netty-socketio-examples/netty-socketio-core-example/src/main/java/com/socketio4j/example/core/CoreExampleMain.java`
github-code-quality[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

github-code-quality[bot]

This comment was marked as resolved.

github-advanced-security[bot]

This comment was marked as resolved.

github-code-quality[bot]

This comment was marked as resolved.

github-code-quality[bot]

This comment was marked as resolved.

github-advanced-security[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 17 additional findings in Devin Review.

Open in Devin Review

@AfterAll
static void stopKafka() {
TestResourceCleanup.runAll("Kafka test container cleanup",
() -> { if (KAFKA != null && KAFKA.isRunning()) KAFKA.close(); });
@AfterAll
static void stopNats() {
TestResourceCleanup.runAll("NATS test container cleanup",
() -> { if (NATS_CONTAINER != null && NATS_CONTAINER.isRunning()) NATS_CONTAINER.stop(); });
() -> { if (node2 != null) node2.stop(); },
() -> { if (kafkaEventStore1 != null) kafkaEventStore1.shutdown(); },
() -> { if (kafkaEventStore2 != null) kafkaEventStore2.shutdown(); },
() -> { if (KAFKA != null && KAFKA.isRunning()) KAFKA.close(); });
Comment on lines +8 to +13
uses: ./.github/workflows/build.yml
with:
javaVersion: "21"
interopVersions: "full"

publish:
okWebSocket.send(ByteString.of(new byte[]{(byte) 0xFF, (byte) 0xFF, (byte) 0xFF}));

// 4. Send valid Engine.IO PING packet to verify server Netty pipeline is still healthy
CountDownLatch pongLatch = new CountDownLatch(1);
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