Improve console implementation #728
Conversation
|
Without having time to comment on the actual implementation, I definitely support the changes presented in this PR. Thanks! |
| + </TerminalConsole> | ||
| <RollingRandomAccessFile name="File" fileName="logs/latest.log" filePattern="logs/%d{yyyy-MM-dd}-%i.log.gz"> | ||
| - <PatternLayout pattern="[%d{HH:mm:ss}] [%t/%level]: %msg%n" /> | ||
| + <PatternLayout pattern="[%d{HH:mm:ss}] [%t/%level]: %minecraftFormatting{%msg}{strip}%n" /> |
|
I have not done a proper review, however from my initial glance through of the proposal I am very interested in seeing this added 👍 |
|
Is there any risks in breaking compat with plugins? Def on board with it though, and clear winner if no breakages are expected and it's one of those "Just makes things better" Demon mentioned option to disable, but I would say let's not waste effort on that and only worry about that IF it becomes a problem. And that would only be the case if the problem isn't fixable. |
|
I could only imagine that some plugins may break if they rely on certain implementation details:
|
| - | ||
| -public class ColouredConsoleSender extends CraftConsoleCommandSender { | ||
| - private final Terminal terminal; | ||
| - private final Map<ChatColor, String> replacements = new EnumMap<ChatColor, String>(ChatColor.class); |
There was a problem hiding this comment.
Id say we should leave this in, so that if a plugin is dumb, it will fail more gracefully (so only the dumb part of the plugin dies)
Also, if upstream changes this file, it would cause conflicts for us.
There was a problem hiding this comment.
Unfortunately, it doesn't compile anymore unless I keep the old dependencies (which are then completely unused). Considering the Paper JAR is already 40 MB I'm not sure if that is a good idea. I could keep in the entire file commented out but I'm not sure if that would help?
There was a problem hiding this comment.
I'd rather see this (and most other deletions) removed entirely rather than commented out. Upstream doesn't change this code frequently and resolving a conflict on a removal is trivial at best.
There was a problem hiding this comment.
Alright. Let me know if I should remove something else that is currently only commented out.
|
|
||
| public class Main { | ||
| - public static boolean useJline = true; | ||
| + //public static boolean useJline = true; // Paper |
There was a problem hiding this comment.
In effort to reduce diff, let's avoid commenting out stuff that won't impact behavior
| } | ||
|
|
||
| - public ConsoleReader getReader() { | ||
| + // Paper start - JLine update |
There was a problem hiding this comment.
In effort to reduce diff, let's avoid commenting out stuff that won't impact behavior
There was a problem hiding this comment.
As below, doesn't compile anymore because the dependency was removed
|
|
||
| entityplayer.spawnIn(world); | ||
| entityplayer.setPosition(loc.getX(), loc.getY(), loc.getZ()); | ||
| - entityplayer.setYawPitch(loc.getYaw(), loc.getPitch()); |
|
|
||
| for (JsonListEntry entry : playerList.getProfileBans().getValues()) { | ||
| result.add(getOfflinePlayer((GameProfile) entry.getKey())); | ||
| - } |
| diff --git a/src/main/java/org/bukkit/craftbukkit/util/TerminalConsoleWriterThread.java b/src/main/java/org/bukkit/craftbukkit/util/TerminalConsoleWriterThread.java | ||
| deleted file mode 100644 | ||
| index b6409711..00000000 | ||
| --- a/src/main/java/org/bukkit/craftbukkit/util/TerminalConsoleWriterThread.java |
There was a problem hiding this comment.
same here, let's avoid deleting the files to keep diff/patch smaller and reduce impact of silly plugins doing silly things.
|
I've removed the patch added by @electronicboy in ca62540 because it's no longer needed with the new JLine version (with my changes, JNA is used instead of jansi). I'm not entirely sure if something similar is needed for JNA, but I guess not? |
stephan-gh
changed the title
|
That patch was only added due to a bug with how jansi extracts it's native component, I have a windows VM (I think) still sitting around that replicates the bug that ca62540 was resolved with, so could consider firing it up to see if we hit some similar stupidity, but we shouldn't* [*] Windows is stupid, especially when it comes to stuff like this, don't blame me if it somehow breaks on somebodies computer. |
There was a problem hiding this comment.
Broadly speaking, I'm quite happy with the implementation you've done here 👍
I hope to test this myself later across some testing servers I maintain locally.
As far as general patch handling goes, you've been extremely conservative with your removals. While I can certainly appreciate that, many of these are not useful and no harder to maintain than just removing the code outright. I commented most instances of this, I may have missed one or two.
These aren't really blockers, they're simple changes I could make on merge. But I'm going to mark as "Request Changes" anyway, just in case someone gets bored.
Good work :)
Thanks again
|
|
||
| - new Thread(new org.bukkit.craftbukkit.util.TerminalConsoleWriterThread(System.out, this.reader)).start(); | ||
| + // Paper start - Handled by TerminalConsoleAppender | ||
| + //new Thread(new org.bukkit.craftbukkit.util.TerminalConsoleWriterThread(System.out, this.reader)).start(); |
There was a problem hiding this comment.
This can be removed entirely.
| try { | ||
| - reader.getTerminal().restore(); | ||
| + // Paper - Use TerminalConsoleAppender | ||
| + // reader.getTerminal().restore(); |
| public void sendMessage(IChatBaseComponent ichatbasecomponent) { | ||
| - MinecraftServer.LOGGER.info(ichatbasecomponent.toPlainText()); | ||
| + // Paper start - Log message with colors | ||
| + //MinecraftServer.LOGGER.info(ichatbasecomponent.toPlainText()); |
|
|
||
| - public int complete(final String buffer, final int cursor, final List<CharSequence> candidates) { | ||
| + // Paper start - Change method signature for JLine update | ||
| + //public int complete(final String buffer, final int cursor, final List<CharSequence> candidates) { |
There was a problem hiding this comment.
Don't need to keep the old signature unless you're interested in backwards compat and want to make that work, keeping it commented doesn't do much for us.
| + // Paper start - Change method signature for JLine update | ||
| + //public int complete(final String buffer, final int cursor, final List<CharSequence> candidates) { | ||
| + public void complete(LineReader reader, ParsedLine line, List<Candidate> candidates) { | ||
| + final CraftServer server = this.server.server; |
There was a problem hiding this comment.
Was this added just so you can stop using this everywhere?
| public org.bukkit.command.ConsoleCommandSender console; | ||
| public org.bukkit.command.RemoteConsoleCommandSender remoteConsole; | ||
| - public ConsoleReader reader; | ||
| + //public ConsoleReader reader; // Paper |
There was a problem hiding this comment.
I do appreciate you keeping entire fields though 👍
| - minecraftserver.reader.addCompleter(new org.bukkit.craftbukkit.command.ConsoleCommandCompleter(minecraftserver.server)); | ||
| + // Paper start - Use TerminalConsoleAppender for colors, command completion is registered in TerminalHandler | ||
| + minecraftserver.console = new com.destroystokyo.paper.console.TerminalConsoleCommandSender(); | ||
| + //minecraftserver.console = org.bukkit.craftbukkit.command.ColouredConsoleSender.getInstance(); |
There was a problem hiding this comment.
All of this can be switched to removal
| import net.minecraft.server.MinecraftServer; | ||
| -import org.fusesource.jansi.AnsiConsole; | ||
| +import net.minecrell.terminalconsole.TerminalConsoleAppender; // Paper | ||
| +//import org.fusesource.jansi.AnsiConsole; // Paper |
There was a problem hiding this comment.
Same as before, no reason to keep imports
|
|
||
| -import jline.console.completer.Completer; | ||
| +// Paper start - JLine update | ||
| +//import jline.console.completer.Completer; |
| try { | ||
| - server.reader.getTerminal().restore(); | ||
| + // Paper start - Use TerminalConsoleAppender | ||
| + //server.reader.getTerminal().restore(); |
|
@Zbob750 Thanks for the review. I wasn't quite sure when to keep the old code and when to remove it. I think I have fixed all your comments. |
| - public int complete(final String buffer, final int cursor, final List<CharSequence> candidates) { | ||
| + // Paper start - Change method signature for JLine update | ||
| + public void complete(LineReader reader, ParsedLine line, List<Candidate> candidates) { | ||
| + final CraftServer server = this.server.server; |
There was a problem hiding this comment.
@Zbob750
Was this added just so you can stop using
thiseverywhere?
No, the command completer is now initialized earlier, directly from the console thread in DedicatedServer. At this point, CraftServer is not available yet so I had to change the field in this class from CraftServer to DedicatedServer. The purpose of this variable is to keep the existing code as original as possible because it references the CraftServer field. It works for some usages (e.g. the ones in the Waitable) but not for the usages prefixed with this..
There was a problem hiding this comment.
Thanks for the explanation 👍
Rewrite console improvements (console colors, tab completion,
persistent input line, ...) using JLine 3.x and TerminalConsoleAppender.
New features:
- Support console colors for Vanilla commands
- Add console colors for warnings and errors
- Server can now be turned off safely using CTRL + C. JLine catches
the signal and the implementation shuts down the server cleanly.
- Support console colors and persistent input line when running in
IntelliJ IDEA
Other changes:
- Update JLine to 3.3.1 (from 2.12.1)
- Server starts 1-2 seconds faster thanks to optimizations in Log4j
configuration
|
Testing:
|
|
I've tested on Linux (Arch Linux). Also tested AMP (so pretty much McMyAdmin) recently on SpongeVanilla which uses the same system. The other panels should work too. |
|
Thanks again 👍 |
| - jline.console.ConsoleReader bufferedreader = reader; | ||
| + // Paper start - Use TerminalConsoleAppender implementation | ||
| + if (com.destroystokyo.paper.console.TerminalHandler.handleCommands(DedicatedServer.this)) return; | ||
| + BufferedReader bufferedreader = new BufferedReader(new InputStreamReader(System.in, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
How to change StandardCharsets.UTF_8? In Windows, any input with cyrillic always appears as ╨┐╨░╨▓╨┐╨▓╨...
There was a problem hiding this comment.
Um, this is what Vanilla Minecraft is using.
There was a problem hiding this comment.
In this case, I don't understand. Server is started in cmd.exe, codepage isn't changed. But instead of "тест" I'm getting "╤В╨╡╤Б╤В"..
There was a problem hiding this comment.
Do you get a > on the input line before the commands? Did this work on previous Paper versions?
There was a problem hiding this comment.
I suggest that you open a new issue about this here: https://github.com/PaperMC/Paper/issues
This is a complete rewrite of CraftBukkit's implementation of various console improvements (console colors, tab completion, persistent input line, ...) using JLine 3.x and TerminalConsoleAppender.
Summary
New features:
Other changes:
New features
Console colors for Vanilla commands
CraftBukkit supports colors in the console for Bukkit commands, but not for the commands included with the Vanilla server. The new implementation handles both, adding colors in various situations:
Click me for screenshots
Add console colors for warnings and errors
I guess this could be something subjective, but in my opinion it makes it much easier to spot warnings and errors in the log because they are now marked in yellow and red. It's entirely optional so I can easily disable it if you don't want it.
Click me for screenshots
Server can now be turned off safely using CTRL + C
Previously this relied entirely on shutdown hooks that are often not given enough time to be able to shut down the server cleanly and save all the changes. JLine will now catch the signal which allows us to shut down the server cleanly.
Console colors and persistent input line when running in IntelliJ IDEA
It seems like this was entirely missing in the CraftBukkit implementation. I don't know how many developers actually run servers directly in their IDE but IntelliJ IDEA has wonderful support for console colors and the persistent input line now:
Click me for screenshots
Implementation details
In my opinion, the old implementation never really survived the switch to Log4J. It was implemented with a Log4J appender included in Mojang's authlib which pushes all messages into a queue that was polled in another thread and then written to the output stream using JLine's API.
TerminalConsoleAppender is a much more simple Log4J appender, which handles the initialization of the JLine terminal and directly writes the messages to the JLine terminal. It checks the environment to detect which features can be supported and enables them if possible. I think the documentation and Javadocs of it are quite well so I'm not going to explain it further.
I've originally written it a few years ago for SpongeVanilla and improved it over the time. An older version of it is also in use by Forge since Minecraft 1.8.8. For Minecraft 1.12 I've decided to refactor it into a separate project which allows other projects to use it easily. I thought I'd try how well it would work for Paper and it seems to work great.
Server startup optimization
TL;DR Paper starts 1 or 2 seconds faster now. This is only a very small part of the PR and yet it took the most space to document (I tried to keep it short, I guess I failed). It's not really relevant for anything above so feel free to skip if you're not interested.
I've mentioned above that the server starts about 1-2 seconds faster with these changes. The reason for this is the way Log4J loads its plugins. Log4J is more or less modular, the core consists mostly out of independent plugins.
There are two ways it can find them at runtime:
Log4J2Plugins.datfile.In the old Log4J version before Minecraft 1.12 the annotation processor was not yet included, so only the log4j-core actually used the
Log4J2Plugins.datfile. Everything was working nicely, though there was a slight performance impact because the packages specified in the log4j configuration had to be scanned for plugins. (For CraftBukkit this is onlycom.mojang.util)In newer log4j versions the metadata file is also generated for custom plugins now, theoretically removing the need for any classpath scanning. However, Mojang run into an issue with the server JAR (see MC-115797) because their shaded JAR now included
Log4J2Plugins.dattwice, once for log4j-core and once for authlib.The solution for that is to merge them, and there are tools available to do this for Maven and Gradle. However, Mojang chose to exclude both, even the one from log4j-core! Log4J handles that gracefully, however the consequence is that is forced to classload literally every single of its classes to check if they are plugins, not only a few classes from custom plugins.
You can easily see that if you enable Log4J's debug logging. It even shows several exceptions because it loads classes with missing dependencies (which are never used in Minecraft or Paper): https://gist.github.com/Minecrell/e39deb871eb63457331a012e0a53b3d1#file-gistfile1-txt-L138-L3273
Except the delay to load and check all the classes included in Log4J it doesn't start causing issues until you add the first custom appender with a
Log4J2Plugins.datfile, which is the case for TerminalConsoleAppender. As soon as oneLog4J2Plugins.datis on the classpath, Log4j will no longer scan its own classes.The solution for this in Paper is extremely simply, all we need to do is to shade log4j-core again to get its plugin map file and merge it with the one from TerminalConsoleAppender. No classpath scanning is necessary at runtime, the delay after
Loading libraries, please wait...is reduced about 1 or 2 seconds (sometimes more).Profiler screenshots
Here is a screenshot from a profiler without
Log4j2Plugins.dat. In this case it takes about 3 seconds to initialize:Now let's add
Log4JPlugins.datand disable all classpath scanning. Now it takes about 800 ms to load instead of 3000ms!: