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

Leaking array buffers #434

Closed
ronag opened this issue Sep 24, 2020 · 15 comments · Fixed by #435
Closed

Leaking array buffers #434

ronag opened this issue Sep 24, 2020 · 15 comments · Fixed by #435
Labels
bug Something isn't working Status: help-wanted This issue/pr is open for contributions

Comments

@ronag
Copy link
Member

ronag commented Sep 24, 2020

Not sure if this is a user, undici, node or V8 bug but I have case where I have lot's of ArrayBuffers allocated and never freed in our Node based proxy:

{rss: 4519784448, heapTotal: 25522176, heapUsed: 20927688, external: 722839852, arrayBuffers: 721332770}

@mcollina @addaleax any hints on how to debug this?

@ronag ronag added bug Something isn't working Status: help-wanted This issue/pr is open for contributions labels Sep 24, 2020
@mcollina
Copy link
Member

Have you got a repro? You can possibly find them using heapdump.
If you need something programmatic to run on servers, use https://github.com/nearform/heap-profiler.

@szmarczak
Copy link
Member

@ronag I remember there was a bug in Node.js 12 related to TLS sockets. What Node.js version are you running?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

14.8, HTTP

Happens at multiple locations with different node versions.

I see that V8 recently did something with GC of ArrayBuffers. Maybe coincidence...

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

I’ll have to continue digging on Monday. Anyway, a FYI in case it affects you.

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

Have you got a repro? You can possibly find them using heapdump.
If you need something programmatic to run on servers, use https://github.com/nearform/heap-profiler.

Does that work with cluster?

@mcollina
Copy link
Member

It might work given some adaptation.

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

Seem to have the same problem on a totally different service which recently started to use undici

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

Been trying to create a repro without success 😞

@szmarczak
Copy link
Member

@ronag How does the undici call look like? What function is used?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

`Client.request``

I think it might be broken back pressure.

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

My current guess it that it has something to do with pause/resume multiple times. This seems to be growing arrayBuffer usage infinitiely:

'use strict'

const { Client } = require('..')
const { createServer } = require('http')
const { Readable } = require('stream')

const requests = 128

const server = createServer()

const limit = 1e9

server.on('request', (req, res) => {
  let bytesWritten = 0

  const interval = setInterval(() => {
    console.log(process.memoryUsage().arrayBuffers / 1e6, bytesWritten, bytesWritten / limit)
  }, 1e3)

  const buf = Buffer.allocUnsafe(16384)
  new Readable({
    read () {
      bytesWritten += buf.length
      this.push(buf)
      if (bytesWritten >= limit) {
        clearInterval(interval)
        this.push(null)
      }
    }
  }).pipe(res)
})

server.listen(0, () => {
  const client = new Client(`http://localhost:${server.address().port}`, {
    pipelining: 1
  })

  for (let n = 0; n < requests; ++n) {
    client.request({ path: '/', method: 'GET', opaque: 'asd' }, (err, data) => {
      if (err) {
        console.error(err)
      }
      data.body
        .resume()
        .on('data', () => {
          data.body.pause()
          setTimeout(() => data.body.resume(), 1e3)
        })
    })
  }
})

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

Could it be that socket.pause() does not work in consume() mode?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

Yep, that's the problem.

ronag added a commit that referenced this issue Sep 24, 2020
ronag added a commit that referenced this issue Sep 24, 2020
ronag added a commit that referenced this issue Sep 24, 2020
@szmarczak
Copy link
Member

@ronag Is it possible to pause a response using client.dispatch?

@ronag
Copy link
Member Author

ronag commented Sep 24, 2020

@ronag Is it possible to pause a response using client.dispatch?

Return false in onBody.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Status: help-wanted This issue/pr is open for contributions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants