Cancenling ProjectileImpactEvent can lead to an infinite loop with piercing arrows

[KnightMiner]

Minecraft Version: 1.18.2, code inspection shows issue still exists on 1.19.3

Forge Version: 40.1.92, verified with code inspection of latest 1.18.2 and 1.19.3 branches

Description of issue:

When canceling ProjectileImpactEvent on a piecing arrow that hit an entity, the game will continue to loop looking for additional piercing entities. However,since the canceled target has likely not moved, they will be selected again as the next closest target likely leading to the event being canceled again and the loop continuing until you either kill the arrow, move it, or give up on canceling the event (effectively making piercing arrows uncancelable without modifying the arrow's position).

Originally discovered in SlimeKnights/TinkersConstruct#5103

Steps to Reproduce:

1. Add a listener to the ProjectileImpactEvent that cancels the projectile under some situation, e..g. when the target is blocking with a shield
2. Fire a piercing arrow at the blocking user. Illagers or PvP are examples that can cause this
3. The game will be stuck in an infinite loop in AbstractArrow

Suggested Fix

The suggested fix would be upon canceling the hit, adding the entity target to piercingIgnoreEntityIds. This means that entity will not receive the event a second time, effectively making them immune to the arrow until its entity list is cleared by vanilla on block hit (which seems reasonable). Fix will likely require a specialized event factory for arrows, or else a larger patch.

Alternative Fix

We could simply break out of the loop when the event is canceled. This will mean that an arrow hitting multiple overlapping entities may be unable to hit one entity simply because another caused the event to cancel, but its hard to describe such a case where this happens.

[sciwhiz12]

Confirmed on 1.19.4-45.0.41 in a Forge development environment through the following steps:

1. Insert the following event handler into ForgeInternalHandler:

@SubscribeEvent
public void onProjectileImpact(ProjectileImpactEvent event) {
    if (event.getRayTraceResult() instanceof EntityHitResult result
            && result.getEntity() instanceof LivingEntity living
            && living.isShiftKeyDown()) {
        event.setCanceled(true);
    }
}

2. Summon a pillager (using a spawn egg).
3. Enchant the pillager's held crossbow using the /enchant command: /enchant @e[type=pillager] minecraft:piercing
4. While in survival mode, sneak while blocking and wait for the pillager to fire an arrow.
   • Observe as the game freezes and stops responding.

[sciwhiz12]

I've been told that the decision on this has been reached previously, which is to rewrite the event from being cancellable to having a custom result enum with three values:

• to carry on with default (vanilla) behavior (DEFAULT);
• to stop at that entity, preventing any piercing effect from working (STOP); and
• to continue on as if the entity was not in the projectile's path, without inflicting damage (CONTINUE).

Contributors are welcome to implement these changes and create a PR.

[sciwhiz12]

As a temporary fix for the event, e6aaaff3f3d764c9dd92c4fc42d0f6e1b62071a5 implements the suggested alternative solution by breaking out of the loop when the event is cancelled. This is only temporary, until someone rewrites the event to be in line with what I said above. We will keep this issue open such a rewrite/permanent solution is merged.