#1413 Don't run onUnregister command in async

src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java

@@ -120,7 +120,8 @@ public class AsynchronousUnregister implements AsynchronousProcess {
         if (player == null || !player.isOnline()) {
             return;
         }
-        commandManager.runCommandsOnUnregister(player);
+        bukkitService.scheduleSyncTaskFromOptionallyAsyncTask(() ->
+            commandManager.runCommandsOnUnregister(player));
 
         if (service.getProperty(RegistrationSettings.FORCE)) {
             teleportationService.teleportOnJoin(player);

src/test/java/fr/xephi/authme/TestHelper.java

@@ -23,6 +23,7 @@ import java.util.logging.Logger;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.BDDMockito.given;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
@@ -134,17 +135,17 @@ public final class TestHelper {
     }
 
     /**
-     * Execute a {@link Runnable} passed to a mock's {@link BukkitService#scheduleSyncTaskFromOptionallyAsyncTask}
+     * Sets a BukkitService mock to run any Runnable it is passed to its method
-     * method. Note that calling this method expects that there be a runnable sent to the method and will fail
+     * {@link BukkitService#scheduleSyncTaskFromOptionallyAsyncTask}.
-     * otherwise.
      *
-     * @param service The mock service
+     * @param bukkitService the mock to set behavior on
      */
-    public static void runSyncTaskFromOptionallyAsyncTask(BukkitService service) {
+    public static void setBukkitServiceToRunOptionallyAsyncTasks(BukkitService bukkitService) {
-        ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+        doAnswer(invocation -> {
-        verify(service).scheduleSyncTaskFromOptionallyAsyncTask(captor.capture());
+            Runnable runnable = invocation.getArgument(0);
-        Runnable runnable = captor.getValue();
+            runnable.run();
-        runnable.run();
+            return null;
+        }).when(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class));
     }
 
     /**

src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java

@@ -22,7 +22,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 
 import java.util.Arrays;
 
-import static fr.xephi.authme.TestHelper.runSyncTaskFromOptionallyAsyncTask;
+import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.mockito.ArgumentMatchers.any;
@@ -161,11 +161,11 @@ public class RegisterAdminCommandTest {
         String kickForAdminRegister = "Admin registered you -- log in again";
         given(commandService.retrieveSingleMessage(MessageKey.KICK_FOR_ADMIN_REGISTER)).willReturn(kickForAdminRegister);
         CommandSender sender = mock(CommandSender.class);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         command.executeCommand(sender, Arrays.asList(user, password));
         TestHelper.runOptionallyAsyncTask(bukkitService);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(validationService).validatePassword(password, user);

src/test/java/fr/xephi/authme/process/unregister/AsynchronousUnregisterTest.java

@@ -13,10 +13,14 @@ import fr.xephi.authme.service.BukkitService;
 import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.service.TeleportationService;
 import fr.xephi.authme.service.bungeecord.BungeeSender;
+import fr.xephi.authme.service.bungeecord.MessageType;
 import fr.xephi.authme.settings.commandconfig.CommandManager;
 import fr.xephi.authme.settings.properties.RegistrationSettings;
+import fr.xephi.authme.settings.properties.RestrictionSettings;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
+import org.bukkit.potion.PotionEffect;
+import org.bukkit.potion.PotionEffectType;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -27,6 +31,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 
 import java.util.function.Function;
 
+import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.mockito.ArgumentMatchers.any;
@@ -89,7 +94,7 @@ public class AsynchronousUnregisterTest {
         // then
         verify(service).send(player, MessageKey.WRONG_PASSWORD);
         verify(passwordSecurity).comparePassword(userPassword, password, name);
-        verifyZeroInteractions(dataSource, limboService, teleportationService, bukkitService);
+        verifyZeroInteractions(dataSource, limboService, teleportationService, bukkitService, bungeeSender);
         verify(player, only()).getName();
     }
 
@@ -108,6 +113,9 @@ public class AsynchronousUnregisterTest {
         given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true);
         given(dataSource.removeAuth(name)).willReturn(true);
         given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true);
+        given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(true);
+        given(service.getProperty(RestrictionSettings.TIMEOUT)).willReturn(21);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         asynchronousUnregister.unregister(player, userPassword);
@@ -118,9 +126,10 @@ public class AsynchronousUnregisterTest {
         verify(dataSource).removeAuth(name);
         verify(playerCache).removePlayer(name);
         verify(teleportationService).teleportOnJoin(player);
-        verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class));
         verifyCalledUnregisterEventFor(player);
         verify(commandManager).runCommandsOnUnregister(player);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
+        verify(player).addPotionEffect(new PotionEffect(PotionEffectType.BLINDNESS, 21 * 20, 2));
     }
 
     @Test
@@ -138,6 +147,8 @@ public class AsynchronousUnregisterTest {
         given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true);
         given(dataSource.removeAuth(name)).willReturn(true);
         given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true);
+        given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(false);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         asynchronousUnregister.unregister(player, userPassword);
@@ -148,9 +159,10 @@ public class AsynchronousUnregisterTest {
         verify(dataSource).removeAuth(name);
         verify(playerCache).removePlayer(name);
         verify(teleportationService).teleportOnJoin(player);
-        verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class));
         verifyCalledUnregisterEventFor(player);
         verify(commandManager).runCommandsOnUnregister(player);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
+        verify(player, never()).addPotionEffect(any(PotionEffect.class));
     }
 
     @Test
@@ -167,6 +179,7 @@ public class AsynchronousUnregisterTest {
         given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true);
         given(dataSource.removeAuth(name)).willReturn(true);
         given(service.getProperty(RegistrationSettings.FORCE)).willReturn(false);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         asynchronousUnregister.unregister(player, userPassword);
@@ -177,8 +190,8 @@ public class AsynchronousUnregisterTest {
         verify(dataSource).removeAuth(name);
         verify(playerCache).removePlayer(name);
         verifyZeroInteractions(teleportationService, limboService);
-        verify(bukkitService, never()).runTask(any(Runnable.class));
         verifyCalledUnregisterEventFor(player);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
         verify(commandManager).runCommandsOnUnregister(player);
     }
 
@@ -203,7 +216,7 @@ public class AsynchronousUnregisterTest {
         verify(passwordSecurity).comparePassword(userPassword, password, name);
         verify(dataSource).removeAuth(name);
         verify(service).send(player, MessageKey.ERROR);
-        verifyZeroInteractions(teleportationService, bukkitService);
+        verifyZeroInteractions(teleportationService, bukkitService, bungeeSender);
     }
 
     @Test
@@ -230,6 +243,7 @@ public class AsynchronousUnregisterTest {
         verify(playerCache).removePlayer(name);
         verifyZeroInteractions(teleportationService);
         verifyCalledUnregisterEventFor(player);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
     }
 
     // Initiator known and Player object available
@@ -241,7 +255,9 @@ public class AsynchronousUnregisterTest {
         given(player.isOnline()).willReturn(true);
         given(dataSource.removeAuth(name)).willReturn(true);
         given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true);
+        given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(false);
         CommandSender initiator = mock(CommandSender.class);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         asynchronousUnregister.adminUnregister(initiator, name, player);
@@ -252,9 +268,9 @@ public class AsynchronousUnregisterTest {
         verify(dataSource).removeAuth(name);
         verify(playerCache).removePlayer(name);
         verify(teleportationService).teleportOnJoin(player);
-        verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class));
         verifyCalledUnregisterEventFor(player);
         verify(commandManager).runCommandsOnUnregister(player);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
     }
 
     @Test
@@ -271,6 +287,7 @@ public class AsynchronousUnregisterTest {
         verify(playerCache).removePlayer(name);
         verifyZeroInteractions(teleportationService);
         verifyCalledUnregisterEventFor(null);
+        verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name);
     }
 
     @Test
@@ -286,7 +303,7 @@ public class AsynchronousUnregisterTest {
         // then
         verify(dataSource).removeAuth(name);
         verify(service).send(initiator, MessageKey.ERROR);
-        verifyZeroInteractions(playerCache, teleportationService, bukkitService);
+        verifyZeroInteractions(playerCache, teleportationService, bukkitService, bungeeSender);
     }
 
     @SuppressWarnings("unchecked")

src/test/java/fr/xephi/authme/service/TeleportationServiceTest.java

@@ -17,14 +17,11 @@ import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
-import org.mockito.invocation.InvocationOnMock;
 import org.mockito.junit.MockitoJUnitRunner;
-import org.mockito.stubbing.Answer;
 
 import java.util.Arrays;
 
-import static fr.xephi.authme.TestHelper.runSyncDelayedTask;
+import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks;
-import static fr.xephi.authme.TestHelper.runSyncTaskFromOptionallyAsyncTask;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
 import static org.mockito.ArgumentMatchers.any;
@@ -92,10 +89,10 @@ public class TeleportationServiceTest {
         given(player.isOnline()).willReturn(true);
         Location firstSpawn = mockLocation();
         given(spawnLoader.getFirstSpawn()).willReturn(firstSpawn);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportNewPlayerToFirstSpawn(player);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(player).teleport(firstSpawn);
@@ -112,10 +109,10 @@ public class TeleportationServiceTest {
         given(player.isOnline()).willReturn(true);
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnJoin(player);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(player).teleport(spawn);
@@ -175,19 +172,16 @@ public class TeleportationServiceTest {
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
         given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true);
-        doAnswer(new Answer<Void>() {
+        doAnswer(invocation -> {
-            @Override
+            SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0];
-            public Void answer(InvocationOnMock invocation) throws Throwable {
+            assertThat(event.getPlayer(), equalTo(player));
-                SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0];
+            event.setTo(null);
-                assertThat(event.getPlayer(), equalTo(player));
+            return null;
-                event.setTo(null);
-                return null;
-            }
         }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class));
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnJoin(player);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(bukkitService).callEvent(any(SpawnTeleportEvent.class));
@@ -201,19 +195,16 @@ public class TeleportationServiceTest {
         Location spawn = mockLocation();
         given(spawnLoader.getSpawnLocation(player)).willReturn(spawn);
         given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true);
-        doAnswer(new Answer<Void>() {
+        doAnswer(invocation -> {
-            @Override
+            SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0];
-            public Void answer(InvocationOnMock invocation) throws Throwable {
+            assertThat(event.getPlayer(), equalTo(player));
-                SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0];
+            event.setCancelled(true);
-                assertThat(event.getPlayer(), equalTo(player));
+            return null;
-                event.setCancelled(true);
-                return null;
-            }
         }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class));
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnJoin(player);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(bukkitService).callEvent(any(SpawnTeleportEvent.class));
@@ -251,10 +242,10 @@ public class TeleportationServiceTest {
         Location limboLocation = mockLocation();
         given(limboLocation.getWorld().getName()).willReturn("forced1");
         given(limbo.getLocation()).willReturn(limboLocation);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnLogin(player, auth, limbo);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(player).teleport(spawn);
@@ -298,10 +289,10 @@ public class TeleportationServiceTest {
         LimboPlayer limbo = mock(LimboPlayer.class);
         Location limboLocation = mockLocation();
         given(limbo.getLocation()).willReturn(limboLocation);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnLogin(player, auth, limbo);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         ArgumentCaptor<Location> locationCaptor = ArgumentCaptor.forClass(Location.class);
@@ -326,10 +317,10 @@ public class TeleportationServiceTest {
         LimboPlayer limbo = mock(LimboPlayer.class);
         Location limboLocation = mockLocation();
         given(limbo.getLocation()).willReturn(limboLocation);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnLogin(player, auth, limbo);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         ArgumentCaptor<Location> locationCaptor = ArgumentCaptor.forClass(Location.class);
@@ -351,10 +342,10 @@ public class TeleportationServiceTest {
         LimboPlayer limbo = mock(LimboPlayer.class);
         Location location = mockLocation();
         given(limbo.getLocation()).willReturn(location);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnLogin(player, auth, limbo);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(player).teleport(location);
@@ -373,10 +364,10 @@ public class TeleportationServiceTest {
         LimboPlayer limbo = mock(LimboPlayer.class);
         Location location = mockLocation();
         given(limbo.getLocation()).willReturn(location);
+        setBukkitServiceToRunOptionallyAsyncTasks(bukkitService);
 
         // when
         teleportationService.teleportOnLogin(player, auth, limbo);
-        runSyncTaskFromOptionallyAsyncTask(bukkitService);
 
         // then
         verify(player).teleport(location);