deno-library / logger

logger for deno
MIT License
24 stars 2 forks source link

Uncaught (in promise) BadResource: Bad resource ID #10

Open NeKzor opened 1 year ago

NeKzor commented 1 year ago

This bug currently crashes the whole process unless you prevent it with unhandledrejection.

Minimal reproducible example:

import Logger from 'https://deno.land/x/logger@v1.1.2/logger.ts';

const logger = new Logger();

await logger.initFileLogger("log", {
    rotate: true,
    maxBytes: 100,
    maxBackupCount: 7,
});

setInterval(() => {
    try {
        logger.info('a');
        logger.info('a');
    } catch (err) { 
        console.error(err);
    }
});

Stacktrace:

error: Uncaught (in promise) BadResource: Bad resource ID
    close(this.file.rid);
    ^
    at Writable.close (https://deno.land/x/logger@v1.1.2/writable.ts:31:5)
    at Writer.write (https://deno.land/x/logger@v1.1.2/writer.ts:46:16)
    at Logger.write (https://deno.land/x/logger@v1.1.2/logger.ts:79:18)
    at Logger.info (https://deno.land/x/logger@v1.1.2/logger.ts:42:12)

Deno version:

deno 1.34.3 (release, x86_64-unknown-linux-gnu)
v8 11.5.150.2
typescript 5.0.4

Unhandled promise: https://github.com/deno-library/logger/blob/0ac21c972d9ff171e1b3bf89de423e3ed532e6ac/logger.ts#L79

Here is another unhandled promise: https://github.com/deno-library/logger/blob/0ac21c972d9ff171e1b3bf89de423e3ed532e6ac/writer.ts#L47

Unrelated but this code here tries to await a sync function: https://github.com/deno-library/logger/blob/0ac21c972d9ff171e1b3bf89de423e3ed532e6ac/logger.ts#L90

fuxingZhang commented 1 year ago

Can you submit a pull request to fix this bug.

UrielCh commented 7 months ago

What your recommended handling for this error ? ignore or try stderr ? or try stderror once, that always better.

NeKzor commented 7 months ago

So how exactly is this issue fixed? Did you even test it?

If you look closely there are two issues here:

1.) You are mixing async and sync code! Make everything async or make everything sync. My guess is that it was supposed to be sync because you designed the logging signature as void. Changing it to Promise is a big breaking change but now you did it anyway and you are still not awaiting every Promise correctly!

2.) The rid problem exists because of a different bug which probably happens because of the file rotation.

As of now there are too many flaws in this library starting with this potential DoS (as it happened on my server). I already moved on to deno std. I don't see why anyone would use this library over std/log.

fuxingZhang commented 7 months ago

So how exactly is this issue fixed? Did you even test it?

If you look closely there are two issues here:

1.) You are mixing async and sync code! Make everything async or make everything sync. My guess is that it was supposed to be sync because you designed the logging signature as void. Changing it to Promise is a big breaking change but now you did it anyway and you are still not awaiting every Promise correctly!

2.) The rid problem exists because of a different bug which probably happens because of the file rotation.

As of now there are too many flaws in this library starting with this potential DoS (as it happened on my server). I already moved on to deno std. I don't see why anyone would use this library over std/log.

Welcome to use Deno Std instead of this library. I have not been working on JS and TS development for over five years, focusing on Go language-related work. The maintenance of this library is currently very limited, and I do not have the ability and energy to do this work well. It relies more on community pull requests.