Server: Upgrade Netty to version 4.1

[StenAL]

The final release of the Netty 3.x line was in 2016 and ever since, it has been EOL (https://netty.io/news/2016/06/29/3-10-6-Final.html). Nowadays it has multiple unpatched security issues. This patch migrates the server to the latest Netty release. 4.1.112. The 4.0 branch is skipped since it is also EOL since 2018.

There are a bunch of breaking changes between the versions:

• Channel attachments were removed in favor of attributes, so now ClientState and Player objects are stored as attributes
• flush needs to be called to actually send messages to clients so all calls to write are replaced with writeAndFlush. In theory this can be optimized to buffer multiple writes but since the server is not performance critical, I opted to keep it simple and always flush writes.
• All Netty imports had their package changed from org.jboss to io.netty
• ChannelBuffer was renamed to ByteBuf
• A bunch of handler methods were renamed (e.g. channelConnected -> channelActive, messageReceived -> channelRead, channelClosed -> channelInactive)
• IdleStateAwareChannelHandler was removed and now idle events are be processed by userEventTriggered. getLastActivityTimeMillis was also removed from idle events, so I reimplemented that functionality in ClientState.
• OneToOneDecoder and OneToOneEncoder were replaced with more strongly typed classes, ByteToMessageDecoder and MessageToByteEncoder. They no longer return values, instead they add to the output list passed in as a parameter.
• The ServerBootstrap API was redesigned to use a fluent API and a ChannelInitializer instead of a pipeline factory. The new API is mostly compatible with the old one though.

[StenAL]

Looks like CI is failing because it can't log in to DockerHub.

[PhilippvK]

Two years ago @pehala also did a migration to Netty 4.1 which was never merged as it was part of a larger WiP PR (#98)

Just for curiosity, it might be worth looking into the changes @pehala did back then for Netty 4.1 and see if it’s meaningful to port them over here.

[PhilippvK]

Looks like CI is failing because it can't log in to DockerHub.

@StenAL Please rebase on https://github.com/PhilippvK/playforia-minigolf/commit/996141ca61cddaa425f69ddb6f44022547d702f1 and try again.

[StenAL]

Rebased, now CI failed with ERROR: invalid tag "//playforia-minigolf:latest": invalid reference format, looks like it wasn't picking up the secrets for Docker registry and namespace. Since the CI step is only building a local image I changed it to just use playforia-minigolf:latest in https://github.com/PhilippvK/playforia-minigolf/pull/114/commits/51cb66bf78a169ca8598c8d250ecb6b8ceeb12a1.

I think https://github.com/PhilippvK/playforia-minigolf/pull/98 looks good and has some nice ideas but there's not much to adapt -- it's a complete rewrite of the client and server whereas this is just a port to a newer version with no functional changes. Maybe in the future I'll try to implement some of the features from there.

[PhilippvK]

@StenAL I already expected the docker image name to cause problems when running on forks. Let’s see if it still works after this is merged.

[pehala]

I think #98 looks good and has some nice ideas but there's not much to adapt -- it's a complete rewrite of the client and server whereas this is just a port to a newer version with no functional changes. Maybe in the future I'll try to implement some of the features from there.

Agreed, this is good to merge with just this change.

Rebased, now CI failed with ERROR: invalid tag "//playforia-minigolf:latest": invalid reference format, looks like it wasn't picking up the secrets for Docker registry and namespace. Since the CI step is only building a local image I changed it to just use playforia-minigolf:latest in 51cb66b.

I would actually extract the CI change out of this PR. I think we can remove building image on PR and only build on push to master, building with maven should be sufficient for PRs

[StenAL]

I think we can remove building image on PR and only build on push to master, building with maven should be sufficient for PRs

I don't quite agree. When I did the upgrade to Java 17, I broke the docker build because I forgot to change the base image. It's nice to build the image on PRs to find out breakages like this, without the step it would only be visible after merging. But I don't feel strongly about this so it's up to you to decide, I can drop the commit if you want to.

[pehala]

I don't quite agree. When I did the upgrade to Java 17, I broke the docker build because I forgot to change the base image. It's nice to build the image on PRs to find out breakages like this, without the step it would only be visible after merging. But I don't feel strongly about this so it's up to you to decide, I can drop the commit if you want to.

Fair enough, how do you feel about separating the jobs for PR and merge and only doing local build without any push or login or anything.

EDIT: my biggest problem is that step for PR and for push is inherently different and I dislike using ifs and other hacky solution, I would rather see it separated

[PhilippvK]

@StenAL 51cb66b is now cherry-picked to master, you can drop it from this PR.

@pehala I also think that building the image locally for pull-requests makes sense. But after merging this, I will move it into another workflow as it can be ran independently to the build.yml

[PhilippvK]

I think we do not need to push the docker image on every commit to master. I will rather move it to the release workflow and only publish an image on new tags. @StenAL @pehala how does this sound to you?

[pehala]

I think we do not need to push the docker image on every commit to master. I will rather move it to the release workflow and only publish an image on new tags.

I like pushing it to :latest on every commit because, well, we do not do releases often, this way there is always a new docker image to use if you want latest & greatest

[StenAL]

I don't feel too strongly but I agree with @pehala, since releases don't happen too often, pushing every commit to :latest is nice. The important part to me is to test building the image in CI for every PR.

I've just rebased the branch and now it's ready for review.

Also, I guess after merging this maybe you should cut a manual release? The last one was 3 years ago and there's been lots of improvements since then.

[PhilippvK]

@StenAL New release is published: https://github.com/PhilippvK/playforia-minigolf/releases/tag/v2.2.0.0-BETA

Thank you for all your commits!