Skip to content

Fix NPE while trying to respawn an already disconnected player#11353

Merged
lynxplay merged 3 commits into
PaperMC:masterfrom
Doc94:fix/11352-spigot-respawn-not-check-connection
Sep 7, 2024
Merged

Fix NPE while trying to respawn an already disconnected player#11353
lynxplay merged 3 commits into
PaperMC:masterfrom
Doc94:fix/11352-spigot-respawn-not-check-connection

Conversation

@Doc94
Copy link
Copy Markdown
Member

@Doc94 Doc94 commented Sep 2, 2024

Closes #11352 by adding a new check for not run the method if the connection is in disconnect state, the method depends in many other methods related to connections then can cause NPE in many places of the respawn logic call the method for things like respawn a player in the disconnection state.

Example related to the issue reported for replicate and test.

@EventHandler
public void onQuit(PlayerQuitEvent event) {
    this.getLogger().info("Try to kill " + event.getPlayer().getName());
    event.getPlayer().setHealth(0);
}

@EventHandler
public void onPlayerDeath(EntityDeathEvent event) {
    if (event.getEntity() instanceof Player player) {
        this.getLogger().info("Try to Respawn " + player.getName());
        player.spigot().respawn();
    }
}

@Doc94 Doc94 requested a review from a team as a code owner September 2, 2024 18:50
@Machine-Maker
Copy link
Copy Markdown
Member

I feel like the "isOnline" method should already be checking this. What about changing the implementation of isOnline to also check this?

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Sep 5, 2024

I feel like the "isOnline" method should already be checking this. What about changing the implementation of isOnline to also check this?

i mean isOnline docs already mention the player can have instances where is online but is disconnected and recommed check other method.. make this change require change the behaviour of that method and can cause "side" effects...

and yeah exists isDisconnected but that check another place and not the method used in respawn then for this case i think is better just check the same thing...

@Lulu13022002
Copy link
Copy Markdown
Contributor

This only fix one specific NPE and not the other ones that might happens on the same var.

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Sep 5, 2024

This only fix one specific NPE and not the other ones that might happens on the same var.

You mean for the connection null? Or i missing another?

@lynxplay lynxplay force-pushed the fix/11352-spigot-respawn-not-check-connection branch from 9939d34 to d7b17e3 Compare September 6, 2024 21:28
@lynxplay lynxplay force-pushed the fix/11352-spigot-respawn-not-check-connection branch from d7b17e3 to 8215760 Compare September 6, 2024 21:44
@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Sep 6, 2024

@Lulu13022002 Moved the diff around, would like your review, I agree that the spigot logic here is rather terrible, but I don't think such a massive diff is nice to maintain for such a scuffed impl.

@Lulu13022002 Lulu13022002 changed the title Fix check for Player#spigot()#respawn() in disconnection state Fix NPE while trying to respawn an already disconnected player Sep 7, 2024
Copy link
Copy Markdown
Contributor

@Lulu13022002 Lulu13022002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, i have renamed the pr now that the original pr has been rewritten completely.
I think the old check can be removed instead of the diff on change. Otherwise there's a typo too.

The npe results in the incorrect call added by our player post respawn
event, which assumes the dimension transition is non-null.

Copies the early return from spigot to not NPE instead of the connection
check in #respawn.
@lynxplay lynxplay force-pushed the fix/11352-spigot-respawn-not-check-connection branch from 8215760 to 6c255a6 Compare September 7, 2024 18:48
@lynxplay lynxplay merged commit 0e82527 into PaperMC:master Sep 7, 2024
@Doc94 Doc94 deleted the fix/11352-spigot-respawn-not-check-connection branch December 23, 2024 15:25
LeonTG pushed a commit to LeonTG/Paper that referenced this pull request May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

Calling "Player#spigot#respawn" in EntityDeathEvent after Logging Out Throws NPE

5 participants