Remove redundant errorCallback from onClose#16
Merged
bghgary merged 3 commits intoBabylonJS:mainfrom Apr 10, 2026
Merged
Conversation
The Java WebSocket library calls onError then onClose on errors. onClose was firing errorCallback again for abnormal close codes, causing double error reporting to the consumer. Added a flag to prevent errorCallback from firing twice. The flag is set in onError so onClose skips the duplicate error call but still fires closeCallback. Abnormal server-initiated closes (without a preceding onError) still get an error callback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a bug in the Java WebSocket wrapper where errorCallback could be invoked twice on a single failure path (library calls onError then onClose).
Changes:
- Introduces an
errorFiredflag to track whetheronErrorhas already emitted an error. - Updates
onCloseto only emiterrorCallbackfor abnormal close codes when an error hasn’t already been reported. - Sets
errorFiredinonErrorbefore callingerrorCallback.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use AtomicBoolean for thread-safe access between onError and onClose - Reset flag in onOpen so reconnections work correctly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The org.java_websocket library already fires onError for error conditions. The wrapper should not re-interpret close codes to generate additional error events. If the library has issues with error reporting, those should be fixed in the library, not worked around in the wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bghgary
changed the title
bkaradzic-microsoft
approved these changes
Apr 10, 2026
bghgary
added a commit
to BabylonJS/JsRuntimeHost
that referenced
this pull request
Apr 13, 2026
Fix WebSocket test flake and update dependencies. ## The Bug Tests called `done()` from both `onerror` and `onclose`, causing mocha's "done() called multiple times" error when the native layer fired both events. ## Root Cause The Windows and Apple WebSocket implementations only fired `onError` on connection failures, not `onClose`. Android's `org.java_websocket` library fires both naturally. This inconsistency meant tests couldn't rely on a single terminal event. ## The Fix **UrlLib** (bumped to 880c257): Added a single `CloseOnce` helper on Windows and `invalidateAndCancelWithCloseCode` on Apple that checks the close code internally — fires `onError` for abnormal codes, then `onClose` exactly once. All platforms now consistently fire `onClose` as the terminal event. **AndroidExtensions** (bumped to 2e85a8d): Removed the redundant `errorCallback` from `onClose` that was re-interpreting close codes. The `org.java_websocket` library already fires `onError` for error conditions. **Tests**: Refactored so `done()` is called exactly once from `onclose` (the terminal event). Errors from earlier phases (assertion failures, `onerror`) are collected in an `error` variable and reported via `done(error)`. Catch blocks call `ws.close()` to ensure `onclose` fires promptly on assertion failure. All `expect` assertions are preserved for property and value checks (`readyState`, `url`, message content). The invalid domain test verifies `onerror` fires before `onclose`. ## Dependencies - BabylonJS/UrlLib#26 - BabylonJS/AndroidExtensions#16 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove the
errorCallbackcall fromonClosethat was re-interpreting close codes to generate error events.The
org.java_websocketlibrary already firesonErrorfor error conditions per the WebSocket spec. The wrapper should just pass through events from the library, not add its own error detection logic on top. If the library has issues with error reporting, those should be fixed in the library, not worked around here.