Fix NPC's are not visible sometimes

...Fixed invisible NPC's by allowing new update tasks to cancel current
tasks instead of cancelling the new task in SkinPacketTracker#updateViewer

cancel packets in SkinPacketTracker#updateViewer after getting PlayerEntry to ensure current scheduled tasks are cancelled.

remove LinkedList, replace with ArrayDeque

ensure EventListen#recalculatePlayer does not execute for NPC's

do a little less work in EventListen.SkinUpdateTracker#shouldUpdate

don't add skinnable entity to pending map if using cache skin - Skin#apply

remove redundant fetch per NPC in Skin; Skin can already fetch once for itself

fix and improve thread safety for profile fetcher; prevent external access
to threads Runnable interface; honor subscriber always invoked from main
thread.

rename ProfileFetchSubscriber to ProfileFetchHandler since subscriber implies the handler will be used continuously

src/main/java/net/citizensnpcs/EventListen.java

@@ -453,6 +453,9 @@ public class EventListen implements Listener {
 
     public void recalculatePlayer(final Player player, long delay, final boolean isInitial) {
 
+        if (player.hasMetadata("NPC"))
+            return;
+
         if (isInitial) {
             skinUpdateTrackers.put(player.getUniqueId(), new SkinUpdateTracker(player));
         }
@@ -492,7 +495,7 @@ public class EventListen implements Listener {
                     && player.getLocation(CACHE_LOCATION)
                         .distanceSquared(npc.getStoredLocation()) < viewDistance) {
 
-                SkinnableEntity skinnable = NMS.getSkinnableNPC(npcEntity);
+                SkinnableEntity skinnable = NMS.getSkinnable(npcEntity);
 
                 results.add(skinnable);
             }
@@ -605,6 +608,8 @@ public class EventListen implements Listener {
             Location currentLoc = player.getLocation(YAW_LOCATION);
             float currentYaw = currentLoc.getYaw();
 
+            if (rotationCount < 2) {
+
                 float rotationDegrees = Setting.NPC_SKIN_ROTATION_UPDATE_DEGREES.asFloat();
 
                 boolean hasRotated =
@@ -612,11 +617,12 @@ public class EventListen implements Listener {
 
                 // update the first 2 times the player rotates. helps load skins around player
                 // after the player logs/teleports.
-            if (hasRotated && rotationCount < 2) {
+                if (hasRotated) {
                     rotationCount++;
                     reset(player);
                     return true;
                 }
+            }
 
             // update every time a player moves a certain distance
             double distance = currentLoc.distanceSquared(this.location);

src/main/java/net/citizensnpcs/commands/NPCCommands.java

@@ -1314,7 +1314,7 @@ public class NPCCommands {
         Messaging.sendTr(sender, Messages.SKIN_SET, npc.getName(), skinName);
         if (npc.isSpawned()) {
 
-            SkinnableEntity skinnable = NMS.getSkinnableNPC(npc.getEntity());
+            SkinnableEntity skinnable = NMS.getSkinnable(npc.getEntity());
             if (skinnable != null) {
                 skinnable.setSkinName(skinName);
             }

src/main/java/net/citizensnpcs/npc/CitizensNPC.java

@@ -189,7 +189,7 @@ public class CitizensNPC extends AbstractNPC {
         boolean couldSpawn = !Util.isLoaded(at) ? false : mcEntity.world.addEntity(mcEntity, SpawnReason.CUSTOM);
 
         // send skin packets, if applicable, before other NMS packets are sent
-        SkinnableEntity skinnable = NMS.getSkinnableNPC(getEntity());
+        SkinnableEntity skinnable = NMS.getSkinnable(getEntity());
         if (skinnable != null) {
             final double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
             skinnable.getSkinTracker().updateNearbyViewers(viewDistance);
@@ -200,7 +200,7 @@ public class CitizensNPC extends AbstractNPC {
                     if (getEntity() == null || !getEntity().isValid())
                         return;
 
-                    SkinnableEntity npc = NMS.getSkinnableNPC(getEntity());
+                    SkinnableEntity npc = NMS.getSkinnable(getEntity());
                     if (npc == null)
                         return;
 

src/main/java/net/citizensnpcs/npc/entity/HumanController.java

@@ -130,7 +130,7 @@ public class HumanController extends AbstractEntityController {
 
         NMS.removeFromWorld(getBukkitEntity());
 
-        SkinnableEntity npc = NMS.getSkinnableNPC(getBukkitEntity());
+        SkinnableEntity npc = NMS.getSkinnable(getBukkitEntity());
         npc.getSkinTracker().onRemoveNPC();
 
         super.remove();

src/main/java/net/citizensnpcs/npc/profile/ProfileFetchHandler.java

@@ -3,7 +3,7 @@ package net.citizensnpcs.npc.profile;
 /**
  * Interface for a subscriber of the results of a profile fetch.
  */
-public interface ProfileFetchSubscriber {
+public interface ProfileFetchHandler {
 
     /**
      * Invoked when a result for a profile is ready.

src/main/java/net/citizensnpcs/npc/profile/ProfileFetchThread.java

@@ -1,9 +1,9 @@
 package net.citizensnpcs.npc.profile;
 
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Deque;
 import java.util.HashMap;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -19,25 +19,15 @@ import org.bukkit.Bukkit;
  *
  * <p>Maintains a cache of profiles so that no profile is ever requested more than once
  * during a single server session.</p>
+ *
+ * @see ProfileFetcher
  */
-public class ProfileFetchThread implements Runnable {
+class ProfileFetchThread implements Runnable {
 
     private final ProfileFetcher profileFetcher = new ProfileFetcher();
-    private final Deque<ProfileRequest> queue = new LinkedList<ProfileRequest>();
+    private final Deque<ProfileRequest> queue = new ArrayDeque<ProfileRequest>();
     private final Map<String, ProfileRequest> requested = new HashMap<String, ProfileRequest>(35);
-    private final Object sync = new Object();
+    private final Object sync = new Object(); // sync for queue & requested fields
-
-    /**
-     * Get the singleton instance.
-     */
-    public static ProfileFetchThread get() {
-        if (PROFILE_THREAD == null) {
-            PROFILE_THREAD = new ProfileFetchThread();
-            Bukkit.getScheduler().runTaskTimerAsynchronously(CitizensAPI.getPlugin(), PROFILE_THREAD,
-                    11, 20);
-        }
-        return PROFILE_THREAD;
-    }
 
     ProfileFetchThread() {}
 
@@ -45,37 +35,36 @@ public class ProfileFetchThread implements Runnable {
      * Fetch a profile.
      *
      * @param name     The name of the player the profile belongs to.
-     * @param subscriber  Optional subscriber to be notified when a result is available.
+     * @param handler  Optional handler to handle result fetch result.
-     *                    Subscriber always invoked from the main thread.
+     *                 Handler always invoked from the main thread.
+     *
+     * @see ProfileFetcher#fetch
      */
-    public void fetch(String name, @Nullable ProfileFetchSubscriber subscriber) {
+    void fetch(String name, @Nullable ProfileFetchHandler handler) {
         Preconditions.checkNotNull(name);
 
-        ProfileRequest request = requested.get(name);
+        name = name.toLowerCase();
-
+        ProfileRequest request;
-        if (request != null) {
-
-            if (subscriber != null) {
-
-                if (request.getResult() == ProfileFetchResult.PENDING) {
-                    request.addSubscriber(subscriber);
-                }
-                else {
-                    subscriber.onResult(request);
-                }
-            }
-
-            return;
-        }
-
-        request = new ProfileRequest(name, subscriber);
 
         synchronized (sync) {
+            request = requested.get(name);
+            if (request == null) {
+                request = new ProfileRequest(name, handler);
                 queue.add(request);
+                requested.put(name, request);
+                return;
+            }
         }
 
-        requested.put(name, request);
+        if (handler != null) {
 
+            if (request.getResult() == ProfileFetchResult.PENDING) {
+                addHandler(request, handler);
+            }
+            else {
+                sendResult(handler, request);
+            }
+        }
     }
 
     @Override
@@ -89,11 +78,38 @@ public class ProfileFetchThread implements Runnable {
                 return;
 
             requests = new ArrayList<ProfileRequest>(queue);
+
             queue.clear();
         }
 
-        profileFetcher.fetch(requests);
+        profileFetcher.fetchRequests(requests);
     }
 
-    private static ProfileFetchThread PROFILE_THREAD;
+    private static void sendResult(final ProfileFetchHandler handler,
+                                   final ProfileRequest request) {
+
+        Bukkit.getScheduler().scheduleSyncDelayedTask(CitizensAPI.getPlugin(),
+                new Runnable() {
+
+                    @Override
+                    public void run() {
+
+                        handler.onResult(request);
+                    }
+                }, 1);
+    }
+
+    private static void addHandler(final ProfileRequest request,
+                                   final ProfileFetchHandler handler) {
+
+        Bukkit.getScheduler().scheduleSyncDelayedTask(CitizensAPI.getPlugin(),
+                new Runnable() {
+
+                    @Override
+                    public void run() {
+
+                        request.addHandler(handler);
+                    }
+                }, 1);
+    }
 }

src/main/java/net/citizensnpcs/npc/profile/ProfileFetcher.java

@@ -1,29 +1,53 @@
 package net.citizensnpcs.npc.profile;
 
+import java.util.Collection;
+import javax.annotation.Nullable;
+
 import com.google.common.base.Preconditions;
 import com.mojang.authlib.Agent;
 import com.mojang.authlib.GameProfile;
 import com.mojang.authlib.GameProfileRepository;
 import com.mojang.authlib.ProfileLookupCallback;
+
+import net.citizensnpcs.api.CitizensAPI;
 import net.citizensnpcs.api.util.Messaging;
 import net.citizensnpcs.util.NMS;
 
-import javax.annotation.Nullable;
+import org.bukkit.Bukkit;
-import java.util.Collection;
 
 /**
  * Fetches game profiles that include skin data from Mojang servers.
  *
  * @see ProfileFetchThread
  */
-class ProfileFetcher {
+public class ProfileFetcher {
+
+    /**
+     * Fetch a profile.
+     *
+     * @param name     The name of the player the profile belongs to.
+     * @param handler  Optional handler to handle the result.
+     *                 Handler always invoked from the main thread.
+     */
+    public static void fetch(String name, @Nullable ProfileFetchHandler handler) {
+        Preconditions.checkNotNull(name);
+
+        if (PROFILE_THREAD == null) {
+            PROFILE_THREAD = new ProfileFetchThread();
+            Bukkit.getScheduler().runTaskTimerAsynchronously(CitizensAPI.getPlugin(), PROFILE_THREAD,
+                    11, 20);
+        }
+        PROFILE_THREAD.fetch(name, handler);
+    }
+
+    ProfileFetcher() {}
 
     /**
      * Fetch one or more profiles.
      *
      * @param requests  The profile requests.
      */
-    public void fetch(final Collection<ProfileRequest> requests) {
+    void fetchRequests(final Collection<ProfileRequest> requests) {
         Preconditions.checkNotNull(requests);
 
         final GameProfileRepository repo = NMS.getGameProfileRepository();
@@ -43,7 +67,7 @@ class ProfileFetcher {
                     public void onProfileLookupFailed(GameProfile profile, Exception e) {
 
                         if (Messaging.isDebugging()) {
-                            Messaging.debug("Profile lookup for skin '" +
+                            Messaging.debug("Profile lookup for player '" +
                                     profile.getName() + "' failed: " + getExceptionMsg(e));
                         }
 
@@ -77,7 +101,7 @@ class ProfileFetcher {
                         } catch (Exception e) {
 
                             if (Messaging.isDebugging()) {
-                                Messaging.debug("Profile lookup for skin '" +
+                                Messaging.debug("Profile lookup for player '" +
                                         profile.getName() + "' failed: " + getExceptionMsg(e));
                             }
 
@@ -125,4 +149,6 @@ class ProfileFetcher {
         return (message != null && message.contains("too many requests"))
                 || (cause != null && cause.contains("too many requests"));
     }
+
+    private static ProfileFetchThread PROFILE_THREAD;
 }

src/main/java/net/citizensnpcs/npc/profile/ProfileRequest.java

@@ -1,13 +1,15 @@
 package net.citizensnpcs.npc.profile;
 
+import java.util.ArrayDeque;
+import java.util.Deque;
+import javax.annotation.Nullable;
+
 import com.google.common.base.Preconditions;
 import com.mojang.authlib.GameProfile;
-import net.citizensnpcs.api.CitizensAPI;
-import org.bukkit.Bukkit;
 
-import javax.annotation.Nullable;
+import net.citizensnpcs.api.CitizensAPI;
-import java.util.ArrayDeque;
+
-import java.util.Deque;
+import org.bukkit.Bukkit;
 
 /**
  * Stores basic information about a single profile used to request
@@ -18,24 +20,24 @@ import java.util.Deque;
 public class ProfileRequest {
 
     private final String playerName;
-    private Deque<ProfileFetchSubscriber> subscribers;
+    private Deque<ProfileFetchHandler> handlers;
     private GameProfile profile;
-    private ProfileFetchResult result = ProfileFetchResult.PENDING;
+    private volatile ProfileFetchResult result = ProfileFetchResult.PENDING;
 
     /**
      * Constructor.
      *
      * @param playerName  The name of the player whose profile is being requested.
-     * @param subscriber  Optional subscriber to be notified when a result is available
+     * @param handler     Optional handler to handle the result for the profile.
-     *                    for the profile. Subscriber always invoked from the main thread.
+     *                    Handler always invoked from the main thread.
      */
-    ProfileRequest(String playerName, @Nullable ProfileFetchSubscriber subscriber) {
+    ProfileRequest(String playerName, @Nullable ProfileFetchHandler handler) {
         Preconditions.checkNotNull(playerName);
 
         this.playerName = playerName;
 
-        if (subscriber != null)
+        if (handler != null)
-            addSubscriber(subscriber);
+            addHandler(handler);
     }
 
     /**
@@ -64,19 +66,24 @@ public class ProfileRequest {
     }
 
     /**
-     * Add a result subscriber to be notified when a result is available.
+     * Add one time result handler.
      *
-     * <p>Subscriber is always invoked from the main thread.</p>
+     * <p>Handler is always invoked from the main thread.</p>
      *
-     * @param subscriber  The subscriber.
+     * @param handler  The result handler.
      */
-    public void addSubscriber(ProfileFetchSubscriber subscriber) {
+    public void addHandler(ProfileFetchHandler handler) {
-        Preconditions.checkNotNull(subscriber);
+        Preconditions.checkNotNull(handler);
 
-        if (subscribers == null)
+        if (result != ProfileFetchResult.PENDING) {
-            subscribers = new ArrayDeque<ProfileFetchSubscriber>();
+            handler.onResult(this);
+            return;
+        }
 
-        subscribers.addLast(subscriber);
+        if (handlers == null)
+            handlers = new ArrayDeque<ProfileFetchHandler>();
+
+        handlers.addLast(handler);
     }
 
     /**
@@ -96,14 +103,14 @@ public class ProfileRequest {
                 ProfileRequest.this.profile = profile;
                 ProfileRequest.this.result = result;
 
-                if (subscribers == null)
+                if (handlers == null)
                     return;
 
-                while (!subscribers.isEmpty()) {
+                while (!handlers.isEmpty()) {
-                    subscribers.removeFirst().onResult(ProfileRequest.this);
+                    handlers.removeFirst().onResult(ProfileRequest.this);
                 }
 
-                subscribers = null;
+                handlers = null;
             }
         });
     }

src/main/java/net/citizensnpcs/npc/skin/PlayerListRemover.java

@@ -35,7 +35,7 @@ public class PlayerListRemover {
 
     /**
      * Send a remove packet to the specified player for the specified
-     * human NPC entity.
+     * skinnable entity.
      *
      * @param player  The player to send the packet to.
      * @param entity  The entity to remove.
@@ -58,6 +58,8 @@ public class PlayerListRemover {
         Preconditions.checkNotNull(player);
 
         PlayerEntry entry = pending.remove(player.getUniqueId());
+        if (entry == null)
+            return;
 
         for (SkinnableEntity entity : entry.toRemove) {
             entity.getSkinTracker().notifyRemovePacketCancelled(player.getUniqueId());
@@ -66,10 +68,10 @@ public class PlayerListRemover {
 
     /**
      * Cancel packets pending to be sent to the specified player
-     * for the specified skinnable NPC.
+     * for the specified skinnable entity.
      *
      * @param player     The player.
-     * @param skinnable  The skinnable NPC.
+     * @param skinnable  The skinnable entity.
      */
     public void cancelPackets(Player player, SkinnableEntity skinnable) {
         Preconditions.checkNotNull(player);
@@ -129,14 +131,13 @@ public class PlayerListRemover {
                 Iterator<SkinnableEntity> skinIterator = entry.toRemove.iterator();
                 while (skinIterator.hasNext()) {
 
+                    if (i >= maxPacketEntries)
+                        break;
+
                     SkinnableEntity skinnable = skinIterator.next();
                     skinnableList.add(skinnable);
 
                     skinIterator.remove();
-
-                    if (i > maxPacketEntries)
-                        break;
-
                     i++;
                 }
 

src/main/java/net/citizensnpcs/npc/skin/Skin.java

@@ -15,8 +15,8 @@ import net.citizensnpcs.Settings;
 import net.citizensnpcs.api.event.DespawnReason;
 import net.citizensnpcs.api.npc.NPC;
 import net.citizensnpcs.npc.profile.ProfileFetchResult;
-import net.citizensnpcs.npc.profile.ProfileFetchSubscriber;
+import net.citizensnpcs.npc.profile.ProfileFetchHandler;
-import net.citizensnpcs.npc.profile.ProfileFetchThread;
+import net.citizensnpcs.npc.profile.ProfileFetcher;
 import net.citizensnpcs.npc.profile.ProfileRequest;
 
 /**
@@ -28,15 +28,15 @@ public class Skin {
     private volatile Property skinData;
     private volatile UUID skinId;
     private volatile boolean isValid = true;
-    private final Map<SkinnableEntity, Void> pending = new WeakHashMap<SkinnableEntity, Void>(30);
+    private final Map<SkinnableEntity, Void> pending = new WeakHashMap<SkinnableEntity, Void>(15);
 
     /**
-     * Get a skin for a human NPC entity.
+     * Get a skin for a skinnable entity.
      *
      * <p>If a Skin instance does not exist, a new one is created and the
      * skin data is automatically fetched.</p>
      *
-     * @param entity  The human NPC entity.
+     * @param entity  The skinnable entity.
      */
     public static Skin get(SkinnableEntity entity) {
         Preconditions.checkNotNull(entity);
@@ -65,13 +65,13 @@ public class Skin {
         this.skinName = skinName.toLowerCase();
 
         synchronized (CACHE) {
-            if (CACHE.containsKey(skinName))
+            if (CACHE.containsKey(this.skinName))
                 throw new IllegalArgumentException("There is already a skin named " + skinName);
 
-            CACHE.put(skinName, this);
+            CACHE.put(this.skinName, this);
         }
 
-        ProfileFetchThread.get().fetch(skinName, new ProfileFetchSubscriber() {
+        ProfileFetcher.fetch(this.skinName, new ProfileFetchHandler() {
 
             @Override
             public void onResult(ProfileRequest request) {
@@ -82,11 +82,8 @@ public class Skin {
                 }
 
                 if (request.getResult() == ProfileFetchResult.SUCCESS) {
-
                     GameProfile profile = request.getProfile();
-
+                    setData(profile);
-                    skinId = profile.getId();
-                    skinData = Iterables.getFirst(profile.getProperties().get("textures"), null);
                 }
             }
         });
@@ -125,47 +122,15 @@ public class Skin {
     }
 
     /**
-     * Set skin data.
+     * Apply the skin data to the specified skinnable entity.
-     *
-     * @param profile  The profile that contains the skin data. If set to null,
-     *                 it's assumed that the skin is not valid.
-     *
-     * @throws IllegalStateException if not invoked from the main thread.
-     * @throws IllegalArgumentException if the profile name does not match the skin data.
-     */
-    public void setData(@Nullable GameProfile profile) {
-
-        if (profile == null) {
-            isValid = false;
-            return;
-        }
-
-        if (!profile.getName().toLowerCase().equals(skinName)) {
-            throw new IllegalArgumentException(
-                    "GameProfile name (" + profile.getName() + ") and "
-                            + "skin name (" + skinName + ") do not match.");
-        }
-
-        skinId = profile.getId();
-        skinData = Iterables.getFirst(profile.getProperties().get("textures"), null);
-
-        for (SkinnableEntity entity : pending.keySet()) {
-            applyAndRespawn(entity);
-        }
-    }
-
-    /**
-     * Apply the skin data to the specified human NPC entity.
      *
      * <p>If invoked before the skin data is ready, the skin is retrieved
      * and the skin is automatically applied to the entity at a later time.</p>
      *
-     * @param entity  The human NPC entity.
+     * @param entity  The skinnable entity.
      *
      * @return  True if the skin data was available and applied, false if
      * the data is being retrieved.
-     *
-     * @throws IllegalStateException if not invoked from the main thread.
      */
     public boolean apply(SkinnableEntity entity) {
         Preconditions.checkNotNull(entity);
@@ -173,7 +138,6 @@ public class Skin {
         NPC npc = entity.getNPC();
 
         if (!hasSkinData()) {
-            pending.put(entity, null);
 
             // Use npc cached skin if available.
             // If npc requires latest skin, cache is used for faster
@@ -202,9 +166,7 @@ public class Skin {
                 }
             }
 
-            // get latest skin
+            pending.put(entity, null);
-            fetchSkinFor(entity);
-
             return false;
         }
 
@@ -233,20 +195,25 @@ public class Skin {
         }
     }
 
-    private void fetchSkinFor(final SkinnableEntity entity) {
+    private void setData(@Nullable GameProfile profile) {
 
-        ProfileFetchThread.get().fetch(skinName, new ProfileFetchSubscriber() {
+        if (profile == null) {
-
+            isValid = false;
-            @Override
-            public void onResult(ProfileRequest request) {
-
-                if (request.getResult() != ProfileFetchResult.SUCCESS)
             return;
-
-                double viewDistance = Settings.Setting.NPC_SKIN_VIEW_DISTANCE.asDouble();
-                entity.getSkinTracker().updateNearbyViewers(viewDistance);
         }
-        });
+
+        if (!profile.getName().toLowerCase().equals(skinName)) {
+            throw new IllegalArgumentException(
+                    "GameProfile name (" + profile.getName() + ") and "
+                            + "skin name (" + skinName + ") do not match.");
+        }
+
+        skinId = profile.getId();
+        skinData = Iterables.getFirst(profile.getProperties().get("textures"), null);
+
+        for (SkinnableEntity entity : pending.keySet()) {
+            applyAndRespawn(entity);
+        }
     }
 
     private static void setNPCSkinData(SkinnableEntity entity,

src/main/java/net/citizensnpcs/npc/skin/SkinPacketTracker.java

@@ -17,6 +17,7 @@ import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
 import org.bukkit.event.Listener;
 import org.bukkit.event.player.PlayerQuitEvent;
+import org.bukkit.scheduler.BukkitTask;
 
 /**
  * Handles and synchronizes add and remove packets for Player type NPC's
@@ -36,7 +37,7 @@ public class SkinPacketTracker {
     /**
      * Constructor.
      *
-     * @param entity  The human NPC entity the instance belongs to.
+     * @param entity  The skinnable entity the instance belongs to.
      */
     public SkinPacketTracker(SkinnableEntity entity) {
         Preconditions.checkNotNull(entity);
@@ -57,48 +58,6 @@ public class SkinPacketTracker {
         return skin;
     }
 
-    /**
-     * Notify that the NPC skin has been changed.
-     */
-    public void notifySkinChange() {
-        this.skin = Skin.get(entity);
-        skin.applyAndRespawn(entity);
-    }
-
-    /**
-     * Notify the tracker that a remove packet has been sent to the
-     * specified player.
-     *
-     * @param playerId  The ID of the player.
-     */
-    void notifyRemovePacketSent(UUID playerId) {
-
-        PlayerEntry entry = inProgress.get(playerId);
-        if (entry == null)
-            return;
-
-        if (entry.removeCount == 0)
-            return;
-
-        entry.removeCount -= 1;
-        if (entry.removeCount == 0) {
-            inProgress.remove(playerId);
-        }
-        else {
-            scheduleRemovePacket(entry);
-        }
-    }
-
-    /**
-     * Notify the tracker that a remove packet has been sent to the
-     * specified player.
-     *
-     * @param playerId  The ID of the player.
-     */
-    void notifyRemovePacketCancelled(UUID playerId) {
-        inProgress.remove(playerId);
-    }
-
     /**
      * Send skin related packets to a player.
      *
@@ -107,19 +66,21 @@ public class SkinPacketTracker {
     public void updateViewer(final Player player) {
         Preconditions.checkNotNull(player);
 
-        if (player.hasMetadata("NPC"))
+        if (isRemoved || player.hasMetadata("NPC"))
             return;
 
-        if (isRemoved || inProgress.containsKey(player.getUniqueId()))
+        PlayerEntry entry = inProgress.get(player.getUniqueId());
-            return;
+        if (entry != null) {
-
+            entry.cancel();
-        PlayerEntry entry = new PlayerEntry(player);
+        }
-        inProgress.put(player.getUniqueId(), entry);
+        else {
+            entry = new PlayerEntry(player);
+        }
 
         PLAYER_LIST_REMOVER.cancelPackets(player, entity);
 
+        inProgress.put(player.getUniqueId(), entry);
         skin.apply(entity);
-
         NMS.sendPlayerListAdd(player, entity.getBukkitEntity());
 
         scheduleRemovePacket(entry, 2);
@@ -175,6 +136,48 @@ public class SkinPacketTracker {
         }
     }
 
+    /**
+     * Notify that the NPC skin has been changed.
+     */
+    public void notifySkinChange() {
+        this.skin = Skin.get(entity);
+        skin.applyAndRespawn(entity);
+    }
+
+    /**
+     * Notify the tracker that a remove packet has been sent to the
+     * specified player.
+     *
+     * @param playerId  The ID of the player.
+     */
+    void notifyRemovePacketSent(UUID playerId) {
+
+        PlayerEntry entry = inProgress.get(playerId);
+        if (entry == null)
+            return;
+
+        if (entry.removeCount == 0)
+            return;
+
+        entry.removeCount -= 1;
+        if (entry.removeCount == 0) {
+            inProgress.remove(playerId);
+        }
+        else {
+            scheduleRemovePacket(entry);
+        }
+    }
+
+    /**
+     * Notify the tracker that a remove packet has been sent to the
+     * specified player.
+     *
+     * @param playerId  The ID of the player.
+     */
+    void notifyRemovePacketCancelled(UUID playerId) {
+        inProgress.remove(playerId);
+    }
+
     private void scheduleRemovePacket(PlayerEntry entry, int count) {
 
         if (!shouldRemoveFromPlayerList())
@@ -189,7 +192,7 @@ public class SkinPacketTracker {
         if (isRemoved)
             return;
 
-        Bukkit.getScheduler().runTaskLater(CitizensAPI.getPlugin(),
+        entry.removeTask = Bukkit.getScheduler().runTaskLater(CitizensAPI.getPlugin(),
                 new Runnable() {
 
                     @Override
@@ -214,9 +217,19 @@ public class SkinPacketTracker {
     private class PlayerEntry {
         Player player;
         int removeCount;
+        BukkitTask removeTask;
+
         PlayerEntry (Player player) {
             this.player = player;
         }
+
+        // cancel previous packet tasks so they do not interfere with
+        // new tasks
+        void cancel() {
+            if (removeTask != null)
+                removeTask.cancel();
+            removeCount = 0;
+        }
     }
 
     private static class PlayerListener implements Listener {

src/main/java/net/citizensnpcs/util/NMS.java

@@ -12,8 +12,10 @@ import java.util.Map;
 import java.util.Random;
 import java.util.Set;
 import java.util.WeakHashMap;
+import javax.annotation.Nullable;
 
 import com.google.common.base.Preconditions;
+import com.mojang.authlib.GameProfile;
 import com.mojang.authlib.GameProfileRepository;
 import com.mojang.authlib.HttpAuthenticationService;
 import com.mojang.authlib.minecraft.MinecraftSessionService;
@@ -21,26 +23,6 @@ import com.mojang.authlib.yggdrasil.YggdrasilAuthenticationService;
 import com.mojang.authlib.yggdrasil.YggdrasilMinecraftSessionService;
 import com.mojang.authlib.yggdrasil.response.MinecraftProfilePropertiesResponse;
 import com.mojang.util.UUIDTypeAdapter;
-import net.citizensnpcs.npc.skin.SkinnableEntity;
-import org.apache.commons.lang.Validate;
-import org.bukkit.Bukkit;
-import org.bukkit.Location;
-import org.bukkit.Material;
-import org.bukkit.Sound;
-import org.bukkit.craftbukkit.v1_8_R3.CraftServer;
-import org.bukkit.craftbukkit.v1_8_R3.CraftSound;
-import org.bukkit.craftbukkit.v1_8_R3.CraftWorld;
-import org.bukkit.craftbukkit.v1_8_R3.entity.CraftEntity;
-import org.bukkit.craftbukkit.v1_8_R3.entity.CraftPlayer;
-import org.bukkit.entity.EntityType;
-import org.bukkit.entity.Horse;
-import org.bukkit.entity.LivingEntity;
-import org.bukkit.entity.Player;
-import org.bukkit.event.entity.CreatureSpawnEvent;
-import org.bukkit.inventory.meta.SkullMeta;
-import org.bukkit.plugin.PluginLoadOrder;
-
-import com.mojang.authlib.GameProfile;
 
 import net.citizensnpcs.api.command.exception.CommandException;
 import net.citizensnpcs.api.npc.NPC;
@@ -48,6 +30,7 @@ import net.citizensnpcs.api.util.Messaging;
 import net.citizensnpcs.npc.ai.NPCHolder;
 import net.citizensnpcs.npc.entity.EntityHumanNPC;
 import net.citizensnpcs.npc.network.EmptyChannel;
+import net.citizensnpcs.npc.skin.SkinnableEntity;
 import net.citizensnpcs.util.nms.PlayerlistTrackerEntry;
 import net.minecraft.server.v1_8_R3.AttributeInstance;
 import net.minecraft.server.v1_8_R3.Block;
@@ -75,7 +58,23 @@ import net.minecraft.server.v1_8_R3.PathfinderGoalSelector;
 import net.minecraft.server.v1_8_R3.World;
 import net.minecraft.server.v1_8_R3.WorldServer;
 
-import javax.annotation.Nullable;
+import org.apache.commons.lang.Validate;
+import org.bukkit.Bukkit;
+import org.bukkit.Location;
+import org.bukkit.Material;
+import org.bukkit.Sound;
+import org.bukkit.craftbukkit.v1_8_R3.CraftServer;
+import org.bukkit.craftbukkit.v1_8_R3.CraftSound;
+import org.bukkit.craftbukkit.v1_8_R3.CraftWorld;
+import org.bukkit.craftbukkit.v1_8_R3.entity.CraftEntity;
+import org.bukkit.craftbukkit.v1_8_R3.entity.CraftPlayer;
+import org.bukkit.entity.EntityType;
+import org.bukkit.entity.Horse;
+import org.bukkit.entity.LivingEntity;
+import org.bukkit.entity.Player;
+import org.bukkit.event.entity.CreatureSpawnEvent;
+import org.bukkit.inventory.meta.SkullMeta;
+import org.bukkit.plugin.PluginLoadOrder;
 
 @SuppressWarnings("unchecked")
 public class NMS {
@@ -108,7 +107,7 @@ public class NMS {
     }
 
     @Nullable
-    public static SkinnableEntity getSkinnableNPC(org.bukkit.entity.Entity entity) {
+    public static SkinnableEntity getSkinnable(org.bukkit.entity.Entity entity) {
         Preconditions.checkNotNull(entity);
 
         Entity nmsEntity = ((CraftEntity) entity).getHandle();
@@ -138,7 +137,8 @@ public class NMS {
                 PacketPlayOutPlayerInfo.EnumPlayerInfoAction.REMOVE_PLAYER, entity));
     }
 
-    public static void sendPlayerListRemove(Player recipient, Collection<? extends SkinnableEntity> skinnableNPCs) {
+    public static void sendPlayerListRemove(Player recipient,
+                                            Collection<? extends SkinnableEntity> skinnableNPCs) {
         Preconditions.checkNotNull(recipient);
         Preconditions.checkNotNull(skinnableNPCs);