feat(diagnostics_channel): add bodyReceived property to trailers

[KhafraDev]

Fixes #1342

This adds a property to the object in the undici:request:trailers called "response" which currently only contains the body received from the server (as an array of buffers).

Downsides:

• Probably uses more memory if subscribed to this event now.

[codecov-commenter]

Codecov Report

Merging #1344 (ef0c4f8) into main (71fd87f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1344      +/-   ##
==========================================
+ Coverage   94.12%   94.13%   +0.01%     
==========================================
  Files          45       45              
  Lines        4205     4214       +9     
==========================================
+ Hits         3958     3967       +9     
  Misses        247      247              

Impacted Files	Coverage Δ
lib/api/api-request.js	100.00% <100.00%> (ø)
lib/core/request.js	98.65% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71fd87f...ef0c4f8. Read the comment docs.

[ronag]

I don’t think this is a good idea. The body can be huge.

[KhafraDev]

Yeah that's fair, would it be better if I created a new stream instead?

[ronag]

Why not just send the body stream as is?

[KhafraDev]

Can't re-use it. It's very helpful for analytics, etc. to be able to see what body was received. For example an API I work with will send 2 types of 429s: a "real" one with JSON and rarely a cloudfare one. When getting a 429, it's more-or-less required to know "which type" it is, as one could be something wrong on your end, and one could be something wrong with a server.

[mcollina]

Unfortunately keeping all the body in memory for all the users of the trailers event is significantly too expensive. Why can't you pass the this.res object to a previous event?

[KhafraDev]

If you consume the body, it can't be consumed later on (I have converted it to a Readable stream, just haven't pushed it yet). Ie:

import diagnosticsChannel from 'diagnostics_channel'
import { request } from 'undici'
import { text } from 'stream/consumers'

diagnosticsChannel.channel('undici:request:trailers').subscribe(({ response }) => {
    // let's say it was the same readable stream
    // consume the body somehow
    text(response.body).then(...)
    // will likely throw because the body has been used already
})

const { body } = await reques('...')
await body.text() // do something with body after

Sending the stream in a previous body wouldn't solve the use case because it would make it work in the channel callback, but would still throw later on in the code

[mcollina]

How about?

import diagnosticsChannel from 'diagnostics_channel'
import { request } from 'undici'
import { text } from 'stream/consumers'

diagnosticsChannel.channel('undici:request:trailers').subscribe(({ response }) => {
   const push = response.push
   response.push = (chunk) => {
     // do whatever you want here
     push.call(response, chunk)
   }
})

const { body } = await reques('...')
await body.text() // do something with body after

[KhafraDev]

After looking into this, we can get the body - as an array of buffers, on the main branch (only change was exposing the underlying stream) - using:

import diagnosticsChannel from 'diagnostics_channel'
import { request } from './index.js'

diagnosticsChannel.channel('undici:request:trailers').subscribe(({ response }) => {
  const kConsume = Object.getOwnPropertySymbols(response).find(s => s.description === 'kConsume');
  console.log(response[kConsume].body);
})

const { body } = await request('https://www.google.com/')
await body.text() // do something with body after

the diff:

diff --git a/lib/core/request.js b/lib/core/request.js
index f04fe4f..3500e46 100644
--- a/lib/core/request.js
+++ b/lib/core/request.js
@@ -216,7 +216,11 @@ class Request {

     this.completed = true
     if (channels.trailers.hasSubscribers) {
-      channels.trailers.publish({ request: this, trailers })
+      channels.trailers.publish({
+        request: this,
+        response: this[kHandler].res,
+        trailers
+      })
     }
     return this[kHandler].onComplete(trailers)
   }

I'm not an expert at node streams nor this part of undici's codebase so I'm very confused as to why this is here...

[KhafraDev]

let body1 = '', body2 = ''
diagnosticsChannel.channel('undici:request:trailers').subscribe(({ response }) => {
  const kConsume = Object.getOwnPropertySymbols(response).find(s => s.description === 'kConsume');
  body2 = Buffer.concat(response[kConsume].body).toString();
})

const { body } = await request('https://www.google.com/')
body1 = await body.text() // do something with body after

console.log(body1 === body2); // true

The bodies are the same 🤯

[KhafraDev]

I have 0 idea how this property works 😅. When fetching stuff on localhost, this property doesn't exist, but when fetching any external server, it's there.. This is making writing tests pretty difficult.

edit: partially figured it out. We have to start consuming the body (body.text(), etc) on a large enough body such that it's consuming before the request finishes.

edit2: That means that this property isn't "technically" reliable, but due to the delay between connecting to an external server, it should never really experience an issue?

We can mimic the behavior on locally hosted servers like

const diagnosticsChannel = require('diagnostics_channel');
const { setTimeout } = require('timers/promises');
const { request } = require('./index');

const server = require('http').createServer((req, res) => {
  res.write('hello')
  setTimeout(10).then(() => res.end('goodbye'))
});

diagnosticsChannel.channel('undici:request:trailers').subscribe(({ response }) => {
  console.log(response.body)
});

server.listen(0, async () => {
  const req = await request(`http://localhost:${server.address().port}`);
  const body = await req.body.text();

  console.log({body})
});

It also means the body must be consumed before the request finishes... Not reliable at all and largely based on timing/server payload. There must be a better way of doing this.

[KhafraDev]

I think the best idea would be to add a new diagnostics channel (maybe undici:body:consumed or something) that sends the entirety of the body once it is consumed.

[mcollina]

That would be a great idea, but it would be possible only for the cases where a body is fully consumed by undici.

[KhafraDev]

I think that's acceptable if it's documented (would fit my usecase perfectly)