Repository navigation
Improve Xbox signaling - #94
SendableMetatype wants to merge 14 commits into
Conversation
|
@rtm516 This might be relevant for your friends of friends work if you plan to keep players in the Xbox session without a TransferPacket |
|
Mostly a port of Kas-tle/NetworkCompatible#7 from July, plus the route probe and fixes found since. |
|
Some of the fixes mentioned in the PR description I'm 99% sure I've added before. If AI was used can you manually re check the changes. |
| super(networkId, xboxToken, signalingUri(networkId)); | ||
| } | ||
|
|
||
| private static URI signalingUri(String networkId) { |
There was a problem hiding this comment.
I'm confused why this is needed. The id passed shouldn't have any characters like this handles to my knowledge.
There was a problem hiding this comment.
I haven't observed it either in practice, but it would theoretically handle other formats, like for e.g specified here: https://github.com/Mojang/bedrock-protocol-docs/blob/c5a73c3b275f3c32366f0c2048643f80e6a13ad8/additional_docs/NetherNetOnboardingGuide.md#9-networkid. I don't see why I would remove it
| * and a registration that died on an open socket with {@link NetherNetXboxRpcSignaling#isRouteAlive(long)}. | ||
| */ | ||
| boolean isActive(); | ||
| boolean isChannelAlive(); |
There was a problem hiding this comment.
What's the reason for this rename?
There was a problem hiding this comment.
No technical reason, it just matches the others and what I originally used. I'll revert if you want
| public static final String XBOX_RPC_INNER_METHOD_WEBRTC = "Signaling_WebRtc_v1_0"; | ||
| public static final String XBOX_RPC_INNER_METHOD_DELIVERY = "Signaling_DeliveryNotification_V1_0"; | ||
| // Our own method, not one the game sends: a host addresses it to itself to test its route | ||
| public static final String XBOX_RPC_INNER_METHOD_ROUTE_PROBE = "RouteProbe_v1_0"; |
There was a problem hiding this comment.
This looks to only be for tests, can we move it to there as it doesn't need to be in releases
There was a problem hiding this comment.
It's not test-only. The RPC signaling sends it to its own player id every 30 seconds and recognizes it when it comes back:
See here:
I saw exactly this happen in practice and this solved it
The PR was an AI assisted port using Claude Opus 5.5 and Gpt-6-Astra routed through claude code, based on my previous implementations. I've reviewed everything, prior to opening and now that you've requested again and nothing that's in here was fully fixed before. Some things things like Kas-tle/NetworkCompatible#8 were already present, but I only touched them if there was an issue that wasn't fully resolved by those alreadyy. That PR for e.g still had a bug where the array branch could never run, because it checked whether the whole message is an array instead of its params. I can trace most of the bigger fixes back to actual issues I had in production |
|
Thanks for this. We hit the One related thing that might be worth folding in: log.error("Error processing signaling frame: " + text, e);Signaling frames can carry SDP/ICE candidates and session details, so we log only the frame's shape instead: log.error("Error processing signaling frame: " + describeFrame(text), e);
// Summarizes untrusted signaling input without copying tokens or session payloads into logs.
private String describeFrame(String text) {
try {
JsonElement parsed = JsonParser.parseString(text);
if (!parsed.isJsonObject()) {
return "root=" + jsonType(parsed);
}
JsonObject frame = parsed.getAsJsonObject();
String method = frame.has("method") && frame.get("method").isJsonPrimitive()
? frame.get("method").getAsString()
: "missing";
return "method=" + method + ", params=" + jsonType(frame.get("params"));
} catch (Exception ignored) {
return "unparseable frame";
}
}
// Reports only a JSON value category so diagnostics cannot leak signaling content.
private String jsonType(JsonElement value) {
if (value == null || value.isJsonNull()) {
return "missing";
}
if (value.isJsonArray()) {
return "array";
}
if (value.isJsonObject()) {
return "object";
}
return "primitive";
}Happy to open it as a separate PR instead if you'd prefer to keep this one focused. |
5b8f174 to
2d65dba
Compare
reconnect(freshToken) replaces only the socket to the signaling service. The handlers, and the server channel and peer connections built on the signaling, stay as they are, so a dropped socket no longer means tearing the transport down. The token is a parameter because a fresh one comes from the application's authentication. A failed attempt leaves the signaling open, so it can be retried. Everything tied to a socket now stays with that socket: - A socket that was replaced can no longer complete, fail or reset the state of its replacement, and its frames are dropped. - The ping loops are bound to their socket and cancelled when it goes inactive, instead of running forever and piling up with every new socket. An exception no longer cancels them silently. - An RPC request fails when its socket closes or after 20 seconds without a reply, instead of waiting forever. A reply only settles a request sent on the same socket. bind() and reconnect() give up after 20 seconds and close the half-open socket, where bind() used to wait for the TURN credentials without a limit. The connect itself no longer blocks while holding the lock. NetherNetXboxRpcSignaling is now Sharable: Netty refuses to add a handler that is not Sharable to a second pipeline, so it could never open a second socket.
A socket whose connection died without closing, for example after a NAT dropped it, still reports itself active, so isActive() could not see it. A WebSocket ping every 15 seconds now obliges the service to answer with a pong, and every received frame, pongs included, records when it arrived. isChannelAlive(maxSilence) is false when nothing arrived within that time, which catches such a socket even when no signals flow.
The WebSocket handler closed the socket on a close frame without saying why. Close frames now reach the signaling, which logs their status code and reason before closing.
TURN credentials were fetched once per socket, so a peer created long after the socket connected could get expired ones. The RPC signaling now fetches them again every 30 minutes and keeps the previous ones if that fails. The legacy signaling applies credentials the service sends at any time, where it used to ignore all but those that completed the connect.
Microsoft's signaling service can keep a socket open after the registration behind it has died. It keeps answering pings, and only closes the socket once it has to route a message through it. Without a probe, that message is the first player who tries to join, so until then the socket looks alive while no join can arrive. Every 30 seconds the RPC signaling now sends a message to its own player id, the pmid claim of the token. The service routes it back only while the registration is alive, over the same route a joining player's signals take, so a dead registration shows without waiting for a player. isRouteAlive(maxSilence) is false when the service refused a probe after an earlier one came back, or when none came back within that time. The probe turns itself off when the token has no pmid, or when the service refuses it before one ever came back, since that shows the service does not accept it rather than that the route is broken. A probe that times out or fails to send proves nothing, and changes nothing.
The check for array or object params tested the request itself, which is always an object, so the array branch never ran. Params sent as an array went to the object branch, where getAsJsonObject throws, and every message in them was lost. The check now tests the params.
Every message received over the RPC signaling was acknowledged with a delivery notification, delivery notifications included. When both ends run this code, as with a proxy joining a host built on it, each notification drew another one back, forever. A delivery notification now ends the exchange.
The legacy signaling put the network id into its URL as it was. Network ids are strings, and one with a slash, a question mark or a dot segment changed which endpoint the socket connected to. The id is now encoded as a single path segment.
The service can split a message across several WebSocket frames, which the aggregator joins back together up to 16 KB. One RPC delivery can carry several messages, so a few offers arriving together can pass that limit. A message over it closes the socket, and with it every join still being signaled over it. Both limits on a message are now 128 KB: the aggregator's for a message split across frames, and the decoder's for one sent as a single frame, which was 64 KB. The size a message may have no longer depends on how the service frames it.
The service answers a request for a player who is not registered with an error, and every such error went to the failure handler. That included the route probe, so on a client whose own registration died, the next probe failed the connection even after the handshake had succeeded. Only signals and delivery notifications, which go to a peer, report a missing peer now. Probes, pings and TURN requests keep their errors on their own request. A request also reports to the failure handler that was set when it was sent, so a handler tied to one connection attempt only hears about that attempt's requests.
parseTurnServers returns what it could read, so a malformed or empty response came out as an empty list and replaced credentials that still worked, and an entry without a URL passed as a server. An update without a server that has a URL now keeps the previous credentials, including the first one on a socket that replaced another. The connect completes either way.
Frames were checked against the current socket under the lock and then processed after it was released, so a reconnect or close could land in between. An old request's reply then went out on the new socket, and a join could still be admitted after close. Socket creation, replacement and close now run on the signaling's own event loop, which also reads the frames, so a frame is always processed in full before the socket changes. No lock is held while handlers or connect observers run, since they can make native calls that wait for a thread sending a signal. Replies go to the socket their request came from. close() only queues that work, since a native callback may call it while the loop waits for that callback. bind() and reconnect() refuse to run on the signaling's own loop, where they would wait for themselves. A failed bind closes the signaling only if no reconnect has replaced its attempt. A subclass that completes connectFuture directly still publishes its ICE servers.
The other signaling tests hand frames straight to the handler, past the pipeline that decides which frames reach it. These run encoded frames through the real upgrade, decoder and aggregator: messages at and over the 128 KB limit in one frame and split across several, pongs between fragments, close reasons, the escaped network id, and buffer release.
Covers a proof that ages past the caller's limit, a returned probe clearing an earlier refusal, and a reconnect resetting both.
3c0e66f to
e68adff
Compare
3 participants
This PR keeps the Xbox signaling working on hosts that run for hours or days. The WebSocket to Microsoft's signaling service can be replaced in place, failures that leave it looking open are detected, TURN credentials stay fresh, and several bugs in the RPC and legacy signaling are fixed.
Changes
reconnect(freshToken)replaces only the socket. The handlers, the server channel and its peers stay, and a failed attempt can be retried.isChannelAlive(maxSilence)catches a socket whose connection died without closing.isRouteAlive(maxSilence)reports whether it still comes back.bind()andreconnect()give up after 20 seconds.Fixes
NetherNetXboxRpcSignalingis@Sharable, so it can open a second socket.For MCXboxBroadcast
It checks
signaling.isActive()and recreates its session when that fails.isActive()only sees a closed socket, so a half-open socket or a dead registration went unnoticed while joins failed. It can now detect both withisChannelAlive(maxSilence)andisRouteAlive(maxSilence), and recover withreconnect(freshToken).API changes
isActive()onNetherNetSignalingand its implementations is renamed toisChannelAlive().close()returns before the socket is closed, andbind()andreconnect()throw when called on its event loop.Tests
New
NetherNetXboxRpcSignalingTest,NetherNetXboxSignalingLifecycleTestandNetherNetXboxWebSocketPipelineTest.