shred / acme4j

Java client for ACME (Let's Encrypt)
https://acme4j.shredzone.org
Apache License 2.0
520 stars 96 forks source link

cert.writeCertificate() not flushing the stream #163

Closed stefanofornari closed 1 month ago

stefanofornari commented 1 month ago

The following code does now work properly (at least for me, Java 19):

Certificate cert = order.getCertificate();

FileWriter out = new FileWriter(new File(preferences.certificate()).getAbsolutePath());
cert.writeCertificate(out);

It basicall does not write the entire content until you flush the stream. maybe it should flush it in AcmeUtils.writeToPem() ?

shred commented 1 month ago

Your writer is not closed after use. The correct way would be:

try (FileWriter out = new FileWriter(new File(preferences.certificate()).getAbsolutePath())) {
    cert.writeCertificate(out);
}

Then the content is fully written. Flushing should not be necessary.

stefanofornari commented 1 month ago

that is indeed true, but as API I think you should take care of it since you do not know when the writer will be closed (the above was just a simple example) Ste

On Sun, Sep 22, 2024 at 12:22 PM Richard Körber @.***> wrote:

Your writer is not closed after use. The correct way would be:

try (FileWriter out = new FileWriter(new File(preferences.certificate()).getAbsolutePath())) { cert.writeCertificate(out); }

Then the content is fully written. Flushing should not be necessary.

— Reply to this email directly, view it on GitHub https://github.com/shred/acme4j/issues/163#issuecomment-2366609425, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKRAV3MJHOISZLQNSHLXDZX2LAHAVCNFSM6AAAAABOUNCLVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRWGYYDSNBSGU . You are receiving this because you authored the thread.Message ID: @.***>

martijnbrinkers commented 1 month ago

No you should close the stream (this is the standard Java use-case). The caller is responsible for closing the Stream. Acme4j cannot close your stream nor would it be a good idea if it could.

stefanofornari commented 1 month ago

of course I should close the stream, I meant you should flush. Ste

On Mon, Sep 23, 2024 at 12:23 PM Martijn Brinkers @.***> wrote:

No you should close the stream (this is the standard Java use-case). The caller is responsible for closing the Stream. Acme4j cannot close your stream nor would it be a good idea if it could.

— Reply to this email directly, view it on GitHub https://github.com/shred/acme4j/issues/163#issuecomment-2367801089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKRASTCBBNTCHRPIHCDLTZX7T3XAVCNFSM6AAAAABOUNCLVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRXHAYDCMBYHE . You are receiving this because you authored the thread.Message ID: @.***>

martijnbrinkers commented 1 month ago

Flush will automatically be called when the stream is closed so there should be no need to call flush.

shred commented 1 month ago

The writer is flushed automatically when closed. I see no use case when preemptively flushing the writer would be helpful. Unless you want to keep the file open on purpose, but then you could invoke out.flush() yourself.

stefanofornari commented 1 month ago

as an API, IMHO, you should not rely on the caller closing the stream; the caller may close it a later stage for several reasons out of your control. The only think under your control is flushing. Anyway, it's your code, so it's your call :) Ste

On Mon, Sep 23, 2024 at 12:29 PM Martijn Brinkers @.***> wrote:

Flush will automatically be called when the stream is closed so there should be no need to call flush.

— Reply to this email directly, view it on GitHub https://github.com/shred/acme4j/issues/163#issuecomment-2367813610, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKRASBFEKGZOKYIPUVAPDZX7UR7AVCNFSM6AAAAABOUNCLVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRXHAYTGNRRGA . You are receiving this because you authored the thread.Message ID: @.***>

shred commented 1 month ago

It is documented in the API that writeCertificate() will not close the writer, and this was a very intentional decision. The stream is provided by the invoker, and so it is not up to the library when to close it. This is especially true since Java provided the try-with-resources statement.

As I said: If you have a very special use case where you want to keep the file open, and want to use it somewhere else even though it's still being written to, you can still invoke flush yourself.

In most cases, the invoker will close the stream immediately, so there is actually no need to preemptively flush it.

I will close this issue now. :wink: