Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better fix for when packets of type HANDSHAKING are not found in the registry. #2945

Closed
wants to merge 2 commits into from

Conversation

nickuc
Copy link
Contributor

@nickuc nickuc commented May 24, 2024

This PR finally fixes the problem that was reported in #2601 introduced by commit af33a2a and replaces the workaround that was previously used.

Based on this code comment:

// since 1.20.5 the encoder is renamed to outbound_config only in the handshake phase
String encoderName = pipeline.get("outbound_config") != null
? "outbound_config" : "encoder";

@derklaro
Copy link
Contributor

I'm wondering: if the key is not present in the handshaking phase, how are packets encoded during that phase? They do need to get the information from somewhere, can this be detected more dynamically than just assuming that it must be handshaking if the key is not present (especially if at some point another protocol is not using the key anymore: the decoding process works but fails with a misleading error + we would have to implement such a detection anyway)

@Ingrim4
Copy link
Collaborator

Ingrim4 commented May 24, 2024

I believe a better solution would be to iterate over the entire pipeline. Since ChannelPipeline implements Iterable<Map.Entry<String, ChannelHandler>>, we can directly check each entry. If we encounter a PacketEncoder or PacketDecoder, we can use them as handler instances. This approach allows us to avoid the need for handler name dependent code.

Edit: this is a partial duplicate of #2933 but I would still prefer my suggested solution since it's more dynamic and less dependent on names not changing.

Maybe something along those lines:

@Override
public Object apply(Channel channel, PacketType.Sender sender) {
    Class<? extends ChannelHandler> handlerClass = this.getHandlerClass(sender)
    		.asSubclass(ChannelHandler.class);
    
    ChannelHandlerContext handlerContext = channel.pipeline().context(handlerClass);
    if (handlerContext == null) {
        return null;
    }

    Function<Object, Object> protocolAccessor = this.getProtocolAccessor(handlerClass, sender);
    return protocolAccessor.apply(handlerContext.handler());
}

private Class<?> getHandlerClass(PacketType.Sender sender) {
	switch (sender) {
		case SERVER:
			return MinecraftReflection.getMinecraftClass("network.PacketEncoder");
		case CLIENT:
			return MinecraftReflection.getMinecraftClass("network.PacketDecoder");
		default:
			throw new IllegalArgumentException("Illegal packet sender " + sender.name());
	}
}

@nickuc
Copy link
Contributor Author

nickuc commented May 24, 2024

I believe a better solution would be to iterate over the entire pipeline. Since ChannelPipeline implements Iterable<Map.Entry<String, ChannelHandler>>, we can directly check each entry. If we encounter a PacketEncoder or PacketDecoder, we can use them as handler instances. This approach allows us to avoid the need for handler name dependent code.

Edit: this is a partial duplicate of #2933 but I would still prefer my suggested solution since it's more dynamic and less dependent on names not changing.

Maybe something along those lines:

@Override
public Object apply(Channel channel, PacketType.Sender sender) {
    Class<? extends ChannelHandler> handlerClass = this.getHandlerClass(sender)
    		.asSubclass(ChannelHandler.class);
    
    ChannelHandlerContext handlerContext = channel.pipeline().context(handlerClass);
    if (handlerContext == null) {
        return null;
    }

    Function<Object, Object> protocolAccessor = this.getProtocolAccessor(handlerClass, sender);
    return protocolAccessor.apply(handlerContext.handler());
}

private Class<?> getHandlerClass(PacketType.Sender sender) {
	switch (sender) {
		case SERVER:
			return MinecraftReflection.getMinecraftClass("network.PacketEncoder");
		case CLIENT:
			return MinecraftReflection.getMinecraftClass("network.PacketDecoder");
		default:
			throw new IllegalArgumentException("Illegal packet sender " + sender.name());
	}
}

I liked your solution. But I'm still having trouble identifying the protocol when the HANDSHAKE packet is received.

I did some tests, and I think I found something:

  • If you call the NettyChannelInjector#getCurrentProtocol(PacketType.Sender.CLIENT) method before the PacketDecoder handler, it correctly returns the HANDSHAKING protocol enum.
  • However, if called after the PacketDecoder handler (where the processInboundPacket is executed), it will return null. Something going on in the internal classes is influencing the behavior of the HANDSHAKING phase, but I haven't figured it out yet.

Oh, I also enabled PR editing, I didn't realize it was disabled.

@nickuc
Copy link
Contributor Author

nickuc commented May 27, 2024

Closed in favor of #2951

@nickuc nickuc closed this May 27, 2024
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