pinojs / pino-socket

🌲 A transport for sending pino logs to network sockets
43 stars 14 forks source link

CI is failing #59

Closed mcollina closed 2 years ago

mcollina commented 2 years ago

See #57 and the other other PRs. I'm a bit strained with time so it would be awesome if somebody else could take a look.

jsumners commented 2 years ago

I know that I technically have all the free time I'm ever going to get to work on stuff, and I'm going to try to do so, but I'm having a hard time doing so. I'll try at some point though.

ramonmulia commented 2 years ago

This issue is related with thread-stream package, I'm not sure how it was working before, but after checking the changes in thread-stream I noticed the following change:

https://github.com/pinojs/thread-stream/commit/ff7c9be4395ac871796773918a385a127c2be52f#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R262

and I believe the following code:

if (this[kImpl].destroyed) {
      if (typeof cb === 'function') {
        process.nextTick(cb, new Error('the worker has exited'))
      }
      return
    }

needs to apply in flushSync and end functions:

Here is where it throws the exception: https://github.com/pinojs/pino/blob/master/lib/transport.js#L65

the functions stream.flushSync() stream.end() are been called when worker is already destroyed but stream is not close.

I tested locally with the changes I proposed and the tests are successfully finished when calling after(function () { setImmediate(process.exit) })

mcollina commented 2 years ago

Would you like to send a PR to thread-stream?

ramonmulia commented 2 years ago

To fix CI, needs a pino with thread-stream@0.15.1, there is a PR opened to update it https://github.com/pinojs/pino/pull/1382

mcollina commented 2 years ago

There is no need to bump the dependency, that's not how semver and npm work.

jsumners commented 2 years ago

Resolved.