Glowstone-Legacy
Glowstone-Legacy copied to clipboard
GlowstoneMC
Switch to Netty ReadTimeoutHandler
The current method we use to timeout clients is based on the server tickrate. We should not use that, and instead use something built-in to Netty to stay reliable.
This is a work in progress, as we should also implement a new method of accomplishing the ping-pong process that is asynchronous. My reasoning behind this is that even if the server stops ticking to do a large amount of logic, your client should not disconnect due to a read timeout like you do in vanilla.
There's still something in flow-net that makes the code behind this PR not entirely uniform. Waiting to see if I can get a solution to that first.
@coelho This is not mergable and still marked as a WIP. Is there something that needs to be done or can this be reviewed fully?
The mergability problem will need to be fixed before review can continue. Thanks!
@ensirius The keep alive packet is used when no data is being sent - IE, we detect that the connection is about to read timeout, so we send the packet.
On a different note, other than the merge conflicts which I'll fix in a bit, and the fact we do an odd cast in GlowSession, this is a pretty solid implementation.
@ensirius If the server is lagging due to a large amount of processing on the world thread, then no, the client will not receive a time update packet every second.
This is why in vanilla Minecraft you get disconnected for read timeouts when the world thread has a large amount of work.
@coelho This is looking pretty inactive code-wise. If after the merge conflicts are resolved this PR is done, please remove "WIP" from the title to have it reviewed.
If this is still a work in progress, please comment back stating so.
Thanks!
I was getting timeouts when doing save-all from console, merging this PR fixed it.
Additionally, I lost 20kg of weight, stopped losing hair, managed to pay off my student loans and got a girlfriend who loves me even though she's not real.
So yeah this PR is pretty good.
"What can I say about the Netty ReadTimeoutHandler that hasn't already been said about the wheel, penicillin, or the iPhone? This is one of the greatest inventions of all time." -- Mrs T.
"I believe that testing this PR has made me a better man, which is remarkable because, well....I'm a chick.* -- Diane P.
"The Netty ReadTimeoutHandler power is obvious. This is living proof that you will get women, and fly. Most importantly my son was born without bones and when I tried this timeout handler with him he grew bones. Don't ask me how it happened but the magic is there. I wish I could hug the author of this PR and thank them for everything they have done for my family" -- Dustin D.
"So I got this ReadTimeoutHandler because of, you know, the sweet timeout handler in it. However, having tested this ReadTimeoutHandler for three weeks now and having tried it out in a variety of situations, both formal and informal, I'm beginning to believe that some of the benefits ---- as described by other reviewers ---- are exaggerated. For example, not ONE supermodel has approached me. Some of you may be used to having supermodels approach you on a regular basis but, believe me, I am not: I would notice one should she appear in my vicinity." -- Go Down M.
"I must admit, that when I first tested this PR, I thought it was the very pinnacle of human achievement. And I was bursting with pride that this great nation created such a marvel. But then, on a missionary trip to Africa, I saw kids so poor, they had no timeout handlers on their servers. No handlers at all. Yet I had a Netty one. I wept in shame.
Upon reflection, I wondered: What must the lesser nations of the world (like Ethiopia, Uganda, France and any of the crappy ones that end in '-slovakia' or '-istan') think of us when they go to their beds at night having no timeout handlers on their servers, while we have the Netty one? Our arrogance must now sting their pride even greater than the long silent whips of their colonial masters. I could sense an underlying envy that hinted at a hidden, yet simmering rage. How could we keep such wealth to ourselves, while others suffered? How could we not share this wonderful innovation with the world?"
-- John F. K.
seriously though merge this please
From the documentation, it seems like IdleStateHandler can perform dual duty for read and write timeouts. Is there a specific reason you chose to have a separate ReadTimeoutHandler?
IdleStateHandler is only for informing the pipeline that a connection has become idle. This functionality is only needed for writes as we use it to send keepalives (to keep the connection open even though we're not sending anything)
As for read timeouts, whenever a timeout hits we basically always close the connection. There's no special behavior that is needed and if there is, something in the pipeline can just catch the exception knowing that the connection is closed. On top of this, IdleStateHandler allows us to disable the read idle portion completely making this really not even affect performance much (if at all).
https://github.com/netty/netty/blob/master/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java#L44
We can always use the IdleStateHandler to replicate the functionality that the ReadTimeoutHandler already provides to us, but I'm not sure if there's a need.
Alright, on further review I better understand exactly what's happening here. However, I still need to consider whether this is the desired behavior. Old behavior will ping the client on read timeout, giving them the opportunity to respond and break the timeout if the client is merely quiet rather than dead (though no compliant client should do this - so perhaps your behavior is better). The write timeout handling can probably be simplified in that case to simply send the ping messages with less extra handling than it currently has, since the old idle logic is to handle read timeouts and all the write timeout is intended to do is prevent the client from reaching a read timeout.
This is mostly an improvement over Vanilla, where in Vanilla if there is a large amount of lag on the main thread, clients will simply disconnect from the server. With this PR, clients will stay connected throughout the lag spike. Although it's not Vanilla behavior, it is definitely more effective than Vanilla.
@SpaceManiac This pull request is pending further review by yourself. Please take the time, if possible, to do so. Thanks!