New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server.createProfile(String name) should not trigger an UUID lookup #881

Closed
Minecrell opened this Issue Mar 26, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@Minecrell
Contributor

Minecrell commented Mar 26, 2018

Problematic code:

@Override
public GlowPlayerProfile createProfile(String name) {
checkNotNull(name);
UUID id = ProfileCache.getUuid(name).join();
if (id == null) {
throw new IllegalArgumentException("Could not fetch UUID for username: " + name);
}
return createProfile(id, name);
}

This should not lookup the UUID for the given name. PlayerProfile is defined to have at least either the UUID or name defined, but not necessarily both. This code may be used with names that don't really exist, and therefore shouldn't trigger an UUID lookup.

An example for such usage is the new PaperServerListPingEvent added in PaperMC/Paper#980. Fake PlayerProfiles may be used to display custom messages in the list of players displayed when hovering the player count in the server list.

I've ported PaperServerListPingEvent to Glowstone in #880, and running ServerListPlus on it then results in a significant delay and the following error: https://gist.github.com/Minecrell/448ad637b2e9d033b1864f522da8d773 because it is looking up the fake profile.

It seems like this was done in #817 because GlowPlayerProfile doesn't support having no UUID defined (see #817 (comment)). However, it should be refactored to support this to match the API specifications.

This is needed for #880.

@Minecrell Minecrell referenced this issue Mar 26, 2018

Merged

Add support for PaperServerListPingEvent #880

3 of 3 tasks complete

@Pr0methean Pr0methean self-assigned this Mar 26, 2018

Pr0methean added a commit to Pr0methean/Glowstone that referenced this issue Mar 26, 2018

GlowPlayerProfile looks up UUID asynchronously
Fixes GlowstoneMC#881. Server.createProfile() now returns immediately, but GlowPlayerProfile.getId() may block.

@wafflebot wafflebot bot added Status: in progress and removed help wanted labels Mar 26, 2018

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Mar 26, 2018

@Minecrell Could you please test #882 as a fix? If it doesn't fix, then it probably at least mitigates the delay. To get a version with the change, go to https://circleci.com/gh/Pr0methean/Glowstone/tree/uuid, click the newest build, then it's under Artifacts.

@Pr0methean Pr0methean changed the title from Server.createProfile(String name) should not trigger an UUID lookup to Server.createProfile(String name) should not block on a UUID lookup Mar 27, 2018

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Mar 27, 2018

@Pr0methean The issue title was correct: it should not trigger an UUID lookup at all. It doesn't matter if it's blocking or not.

Your fix improves the situation somewhat, because it no longer throws an exception if the UUID cannot be found. But it's not the correct fix: It will still block for ~250ms on the first run because the code in PaperServerListPingEvent calls getId() on the added profiles (and expects them to return null eventually).

09:35:32 [WARNING] Couldn't get UUID due to IO error: java.io.IOException: Server returned HTTP response code: 400 for URL: https://api.mojang.com/profiles/minecraft

The PlayerProfile shouldn't be (partially) completed in a potentially blocking operation unless one of the complete() methods is called. If it was created without UUID/without name, the getters should return null, until it gets completed manually by calling complete().

@Minecrell Minecrell changed the title from Server.createProfile(String name) should not block on a UUID lookup to Server.createProfile(String name) should not trigger an UUID lookup Mar 27, 2018

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Mar 27, 2018

Please re-open this issue. As mentioned above, the issue is not fixed in #882.

@momothereal momothereal reopened this Mar 27, 2018

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Mar 27, 2018

@Minecrell Is there some reason not to do the lookup eagerly, once getId() is made non-blocking? The worst case is that we waste a few bytes of bandwidth and, if fully utilizing the CPU, slightly delay another async task on ForkJoinPool.commonPool() (which is where the UUID lookups run, since they use CompletableFuture.supplyAsync).

Pr0methean added a commit to Pr0methean/Glowstone that referenced this issue Mar 27, 2018

@mastercoms mastercoms closed this Mar 27, 2018

@mastercoms mastercoms reopened this Mar 27, 2018

@mastercoms

This comment has been minimized.

Member

mastercoms commented Mar 27, 2018

@Pr0methean I believe it's for fake players where this is no point to looking it up. As stated, complete() can be called if the profile needs to be looked up and completed.

@Minecrell

This comment has been minimized.

Contributor

Minecrell commented Mar 28, 2018

@mastercoms is right, but in this case, the usage doesn't really matter. This is just about implementing the API correctly (granted, the Javadocs on some methods could be more clear).

Server.createProfile(...) should take the given parameters and create a PlayerProfile with them. No lookup or anything, just create a profile with the given values.

PlayerProfile.complete() (blocking) or PlayerProfile.completeCached() (not blocking, should only use cached values) can then be called additionally by plugins to complete the remaining parts, i.e. UUID/name, textures etc.

The main question is if having GlowPlayerProfile.getId() nullable will cause problems in other places of the Glowstone code. If that is the case, it might be necessary to add some checks and eventually call .complete() where necessary. This is why I didn't create a PR to fix this, I'm not familiar with all the other usages of GlowPlayerProfile in Glowstone.

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Mar 29, 2018

I'll overload GlowServer.createProfile() so that the async completion is optional, and modify the existing calls to enable it.

Pr0methean added a commit to Pr0methean/Glowstone that referenced this issue Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment