Fix packet duplicating at some points

[sandtechnology]

Overview: Due to the weakly consistent of ConcurrentLinkedQueue iterator, at some points, packet will be resent twice times or more, causing some weird behaviors (e.g. kicked for illegal movement since the same ClientboundPlayerPositionPacket was sent two times). This changes for the patch add a flag for marking if the packet was consumed to prevent such issue and ensure consistently of the packet queue.

Related issue: https://github.com/PotatoCraft-Studio/QuickShop-Reremake/issues/255 https://github.com/Ghost-chu/QuickShop-Hikari/issues/517

Related logs (By adding debug log): https://pastebin.com/eqfsz2kn

Paper build for testing (Updated): https://mega.nz/file/BwA3HC7I#qZgM8b93X0NKiidE3prUFZkg7aqdetgS3jiLgu8r-K4

Additional Information: Spigot does not have this issue because the original one is using poll method, and paper is using iterator for anti-xray feature.

[Ghost-chu]

I can confirm this bug cause user kicked by reason Invalid move player packet received.

Everything works properly with patched Paper build.

also may be related to https://github.com/PaperMC/Paper/issues/8222

[electronicboy]

Changes fixing issues induced by patches should go into the original patch, rather than as an extra patch down the line;

I'm also not fond of this fix as it's not really fixing the underlying issue, really, that loop needs changing back into a peek/poll loop which will properly manage the queue rather than trying to mangle it with state flags

[sandtechnology]

Changes fixing issues induced by patches should go into the original patch, rather than as an extra patch down the line;

I'm also not fond of this fix as it's not really fixing the underlying issue, really, that loop needs changing back into a peek/poll loop which will properly manage the queue rather than trying to mangle it with state flags

Thanks for the suggestion! I will made this change to previous patches and change it back to poll loop, and seems this change conflicted with couples patches, it will need some time to done.

[Spottedleaf]

The poll loop will not fix this either. There's no way to use CLQ with some kind of conditional poll logic, which is why the iterator was used. However, the iterator doesn't actually fix the issue as it does not indicate to the caller whether the node was actually removed by the remove() call or was already removed.

You can fix this issue by using an AtomicBoolean on the PacketHolder class and use getAndSet(true) to atomically mark a packet as sent and check if it was already sent.

[sandtechnology]

Originally I want to use an variable with mutex to save the unready packet and restore to poll loop, but I found it's bad for readability and maintainability, because it needs change all packet queue related stuff to made it behave normally, which brings more code changes and more complex than the original one.

So I choose just merge current changes to original patch and apply the suggestion from Spottedleaf (Thanks for it!), change the variable type to AtomicBoolean, also think more about muti-threading - let it become the isConsumed flag and use compareAndSet to ensure don't be consumed twice times, it should be looks good enough now.

Test jar will be updated later, it needs sometime to build.

[sandtechnology]

Emmmm, look like the patch file is broken for incorrectly conflict solving, will fix it later

[sandtechnology]

Tested with the newest changes and seems still work for me, now this PR is ready for review again, paper build also was updated for testing.

[sandtechnology]

It has been 9 days, When this PR will be merged? Does it require me to do any extra things?

[xism4]

It has been 9 days, When this PR will be merged? Does it require me to do any extra things?

They aproved it, probably they're just waiting for some other reviewer?

[tillhfm]

I had a short convo with a paper mod on Sunday about this issue because I need it to be merged too and they said "Can be next hour or in 2023, we cant really tell" but I don't know why

[MiniDigger]

we are volunteers, we do this in our free time, we can't ever give estimates on anything. we have a giant queue of open PRs that the team is slowly churning thru now (first time below 200 in a looong time!), but it takes time. on the status of this PR I can say that cat gave this an initial look and concluded that this looks like a good change that we want to have in paper, leaf had some implementation suggestings that seems to have been applied and its now waiting for somebody else on the team to dive into the code, give it a closer look and test it, before it can then be merged. owen added the fancy build-pr-jar label, that means anybody else can help testing too and report any findings here!

[Owen1212055]

In the future, please open PRS on other branches as the master branch makes it a little harder for us to work with your PR. 😄

[sandtechnology]

In the future, please open PRS on other branches as the master branch makes it a little harder for us to work with your PR. 😄

Oh, sorry for that, I will create the another branch other than master when opening another PRs.

[Owen1212055]

No worries, thank you for your contribution regardless.