jpos / jPOS

jPOS Project
http://jpos.org
GNU Affero General Public License v3.0
604 stars 459 forks source link

Log folder does not exist, causing silent failure at start up #603

Open mchhil-incomm opened 1 month ago

mchhil-incomm commented 1 month ago

If the logName is like xyx/a.log and the xyz folder does not exist a FileNotFound exception is thrown and since the log file is not there it gets lost.

https://github.com/jpos/jPOS/blob/master/jpos/src/main/java/org/jpos/util/RotateLogListener.java#L154-L161

I have made a local code change to create the path directories

    protected synchronized void openLogFile() throws IOException {
        if (f != null) {
            f.close();
        }

        Path filePath = Paths.get(logName);
        if (!(filePath.toFile()
                      .exists())) {
            // Ensure the parent directories exist
            Files.createDirectories(filePath.getParent());
            // Create the file if it does not exist
            Files.createFile(filePath);

        }

        f = new FileOutputStream(filePath.toFile(), true);

     setPrintStream (new PrintStream(f)); 
     p.println ("<?xml version=\"1.0\" encoding=\"UTF-8\"?>"); 
     p.println ("<logger class=\"" + getClass().getName() + "\">"); 
    }

And a unit test for my environment where the logger deploy has the appropriate prefix suffix

  <log-listener class="org.jpos.util.DailyLogListener">
    <property name="prefix" value="xyx/a" />
    <property name="suffix" value=".log" />
    <property name="window" value="86400" />
    <property name="copies" value="90" />
    <property name="maxsize" value="100000000" />
    <property name="compression-format" value="none" />
    <property name="rotate-on-startup" value="true" />
  </log-listener>

    public void initSwitchTest() throws InterruptedException, IOException {

        Q2 q2 = new Q2("deploy");
        q2.start();

        Awaitility.await().atMost(10, SECONDS).until(q2::running);
        Thread.sleep(5000);

        q2.shutdown();
        Assert.assertTrue(new File("xyx/a.log").exists());
    }
ar commented 1 month ago

I have mixed feelings here. This doesn't solve another very common situation. A user runs as root once (because you know, that what happens most of the time with non Unix people), and then when the system runs as a regular user, it still can't write the log, and you have the same visibility problem.

Wouldn't it be better to just bubble up an exception so that the logger configuration gets renamed as .BAD raising some visibility on the problem?

If there's a typo in the log path, we still want to correct it instead of creating a new directory somewhere, perhaps out of a volume or filesystem we really want.

As always, I like to be proven wrong (which is usually an easy task :) so I'm listening to your comments.

mchhil-incomm commented 1 month ago

My opinion is attempt the directory and file creation, if it still fails make the logger a .BAD and do a printstacktrace. printstacktrace because you may not have redirected the standard outs to the q2 log which can help with debugging.

My case is a little weirds we write different logs in addition to q2 to the log directory, the others create the log folder (logback and jetty), so down the line the q2 starts getting written there as the log folder is now available, though initially it failed.

In my test case I create the log (or xyz as in the example) folder in the unit test before starting q2, so we worked around it.

alcarraz commented 1 month ago

What if we add a property for that?

So the dev/maintainer has a choice. It can be false in prod and true in dev or test envs via Environment properties or different configurations

mchhil-incomm commented 1 month ago

Property would work, it's the easiest way to keep things backwards compatible and move forward with updated development.