Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature idea] Introduce a FIFO queue with a max size to store data when the TCP socket write is unsuccessful #72

Closed
ggrossetie opened this issue Jul 8, 2022 · 3 comments · Fixed by #74

Comments

@ggrossetie
Copy link
Contributor

The goal is to avoid "losing" data/logs when the TCP server is unavailable (for a short period of time). We should configure a max size to prevent unsafe unbounded storage (i.e. exhausting the memory).

To give you an idea of what I'm after, here's a naive implementation:

write (data, encoding, callback) {
  socket.write(data, encoding, (err) => {
    if (err) {
      // unable to write data, will try later when the server becomes available again
      queue.push({data, encoding})
    }
  })
  callback()
}

And in the reconnect/retry:

retry.on('ready', () => {
  connect((err) => {
    if (connected === false) {
      return retry.backoff(err)
    } else if (!queue.isEmpty()) {
      const items = queue.items()
      while (items.length > 0) {
        const { data, encoding } = items.shift()
        outputStream.write(data, encoding)
      }
    }
  })
})
@mcollina
Copy link
Member

mcollina commented Jul 9, 2022

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@ggrossetie
Copy link
Contributor Author

Yes, I can send a pull request for this feature!

Do you have any preference regarding the FIFO queue implementation? Should we use an existing library? Should we use our own implementation to avoid adding a new dependency?

@mcollina
Copy link
Member

mcollina commented Jul 9, 2022

I'll leave that to you and then review when a PR is open!

ggrossetie added a commit to ggrossetie/pino-socket that referenced this issue Jul 9, 2022
ggrossetie added a commit to ggrossetie/pino-socket that referenced this issue Jul 9, 2022
ggrossetie added a commit to ggrossetie/pino-socket that referenced this issue Jul 9, 2022
ggrossetie added a commit to ggrossetie/pino-socket that referenced this issue Jul 18, 2022
mcollina pushed a commit that referenced this issue Jul 21, 2022
* resolves #72 introduce a recovery queue

* remove suite

* ensure that no error is thrown

* default maxSize 1024 and remove arbitrary setTimeout

* add documentation

* copy editing and emit a warning when recovery failed

* fix typo in comment

* events consistency and update CLI

* improve doc

* copy editing

* fix for node 14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants