ZeNyfh / gigavibe-java-edition

Music + Media bot made in java using JDA and Lavaplayer.
GNU General Public License v3.0
8 stars 3 forks source link

Faulty newline handling for printf in OutputLogger #167

Closed WolfNT90 closed 1 month ago

WolfNT90 commented 1 month ago

Describe the bug During development for pr-autoplay, I stumbled upon a bug with the OutputLogger implementation.

To Reproduce Steps to reproduce the behavior:

  1. On main, add 3 lines of System.out|err.printf(arg0, arg1 ...); to the end of the main method, including a newline %n at the beginning, or somewhere in the format arg. 1A. My exact snippet:
    System.out.printf("%n "+System.currentTimeMillis()+" Test.");
    System.out.printf("%n "+System.currentTimeMillis()+" Test.");
    System.out.printf("%n "+System.currentTimeMillis()+" Test.");
  2. See
    2024/05/11 23:10:48 | bot is now running, have fun ig
    2024/05/11 23:10:48 | 
    2024/05/11 23:10:48 |  1715458248436 Test.2024/05/11 23:10:48 | 
    2024/05/11 23:10:48 |  1715458248438 Test.2024/05/11 23:10:48 | 
    2024/05/11 23:10:48 |  1715458248438 Test.

Expected behavior On a new, clear Class, I get the following output.

 1715458365330 Test.
 1715458365339 Test.
 1715458365340 Test.

Screenshots Logs attached instead

Additional context I was going to use printf so that I don't have to String.format( ... ); a regular sysout, but, it borks!

9382 commented 1 month ago

I think the main issue here is that printf is complex and submits to out in parts but the OutputLogger is naive (or stupid depending on how you feel about it) and thinks that the message is "a different one" for each of its segments because its logic is very basic and therefore requires a timestamp.

Also, your usage of printf is confusing. Using a different example with proper args (System.out.printf("%s Test.", System.currentTimeMillis()); 3 times) creates 2024/05/11 22:13:21 | 17154620014562024/05/11 22:13:21 | Test.2024/05/11 22:13:21 | 17154620014672024/05/11 22:13:21 | Test.2024/05/11 22:13:21 | 17154620014672024/05/11 22:13:21 | Test. (you can clearly see it timestamping every "split" in the format)

I do have a potential solution in mind to mitigate this and am working on it right now

WolfNT90 commented 1 month ago

Also, your usage of printf is confusing.

I realized later that I should've probably used it as I intended to use it, but I think my thought process must've been to simply showcase that any printf is faulty. My bad.

9382 commented 1 month ago

The new version (3733765) will now stop adding timestamps onto prints that dont terminate in newlines. Newlines that are part of the same print (and that get received by the logger in the same byte array, aka not printf's %n) will continue to only be timestamped at the start of the entire string (e.g. println("a\na\na")), which isn't great and this should probably be improved, but is no different from the old behaviour and is different to this specific bug

9382 commented 1 month ago

I've improved it further with 3a9a7ca (in functionality, the code itself looks a bit bad) to fix the behaviour I describe in my above comment. If there's any further issues (or new ones caused by this change that I haven't spotted), feel free to re-open this issue