Buffer.from doesn't allow "null" chunk

[thib3113]

What is the expected behavior?
no error

What is the actual behavior?
In some case, an error is Throw :

    TypeError : The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received null 
     TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received null
        at Function.from (buffer.js:313:9)
        [...]

Possible solution
here you check if typeof chunk !== 'undefined', but typeof null === 'object' .
So, the chunk pass in recordChunk, and recordChunk try to do Buffer.from(chunk, [...]), and chunk is null.

How to reproduce the issue
I doesn't know how I produce it (for the moment), but It seems to be a little check

Having problem producing a test case? Try and ask the community for help. If the test case cannot be reproduced, the Nock community might not be able to help you.

Does the bug have a test case?
no
Versions

Software	Version(s)
Nock	12.0.2
Node	13.8.0
TypeScript	3.8.3

[mastermatt]

Are you calling request.write(null) before request.end()? I'm able to get the same error by doing that, but that is not a valid call.

If request.write(null) is called, without Nock in the mix, you get the following error from this check in Node.

TypeError [ERR_INVALID_ARG_TYPE]: The first argument must be one of type string or Buffer. Received type object

So when Nock is in record mode, the error messaging is slightly different, however, erring in general is the correct action.
@thib3113 can you confirm if you're getting this error another way.

[thib3113]

In fact, I'm just using axios ... I Doesn't call req directly .

And I run tests with jest (with nodejs http resquest), and I've two tests that produce this error, but Doesn't produce the error if run standalone ....

[mastermatt]

Can you provide a simple runkit example that uses axios and nock to recreate the error?

[thib3113]

@mastermatt as I say, running my test alone doesn't produce the error, the error is only throw when I run all my tests ... ( and, it doesn't throw the error last time )

So, I'll need to do some search to find what is really the problem, and why I receive null here ... I will try some things to find the error when back at work (monday)

[thib3113]

Ok, so Some informations (I'm doing some search) .

I update recordChunk function in Recorder.js

      const recordChunk = (chunk, encoding) => {
      debug(thisRecordingId, 'new', proto, 'body chunk')
        if(chunk === null){
            debug(thisRecordingId,proto, 'body chunk is null', new Error());
            return;
        }
      if (!Buffer.isBuffer(chunk)) {
        chunk = Buffer.from(chunk, encoding)
      }
      bodyChunks.push(chunk)
    }

And, here is my trace

  nock.recorder 1 new http body chunk +0ms
  nock.recorder 1 http body chunk is null Error: 
    at recordChunk (C:\Users\thib3113\repos\project\node_modules\nock\lib\recorder.js:326:64)
    at OverriddenClientRequest.req.write (C:\Users\thib3113\repos\project\node_modules\nock\lib\recorder.js:338:9)
    at InterceptedRequestRouter.handleEnd (C:\Users\thib3113\repos\project\node_modules\nock\lib\intercepted_request_router.js:154:11)
    at OverriddenClientRequest.req.end (C:\Users\thib3113\repos\project\node_modules\nock\lib\intercepted_request_router.js:77:33)
    at OverriddenClientRequest.req.end (C:\Users\thib3113\repos\project\node_modules\nock\lib\recorder.js:362:14)
    at C:\Users\thib3113\repos\project\node_modules\follow-redirects\index.js:119:20
    at RedirectableRequest.write (C:\Users\thib3113\repos\project\node_modules\follow-redirects\index.js:85:7)
    at RedirectableRequest.end (C:\Users\thib3113\repos\project\node_modules\follow-redirects\index.js:118:8)
    at dispatchHttpRequest (C:\Users\thib3113\repos\project\node_modules\axios\lib\adapters\http.js:276:11)
    at new Promise (<anonymous>)
    at httpAdapter (C:\Users\thib3113\repos\project\node_modules\axios\lib\adapters\http.js:21:10)
    at dispatchRequest (C:\Users\thib3113\repos\project\node_modules\axios\lib\core\dispatchRequest.js:52:10)
    at HydraList.loadPage (C:\Users\thib3113\repos\project\src\objects\Lists\HydraList.ts:246:17)
    at C:\Users\thib3113\repos\project\src\objects\Lists\HydraList.ts:144:29 +0ms

And in the trace, we see this :

at RedirectableRequest.end (C:\Users\thib3113\repos\project\node_modules\follow-redirects\index.js:116:8)

And this line is :

// Ends the current native request
RedirectableRequest.prototype.end = function (data, encoding, callback) {
  // Shift parameters if necessary
  if (typeof data === "function") {
    callback = data;
    data = encoding = null;
  }
  else if (typeof encoding === "function") {
    callback = encoding;
    encoding = null;
  }

  // Write data and end
  var currentRequest = this._currentRequest;
  console.log('follow-redirects/index.js');
  console.log(data || "", (data || "").length);
  this.write(data || "", encoding, function () {
    currentRequest.end(null, null, callback); //119
  });
};

So, it seems that this.write is called before req.end ? no ?
Did you have any idea ? ( it seems my tests doesn't bug each times ... without modifications ) ( but I do real request, with recording them, so maybe the server doesn't return the same things )

[thib3113]

Ok, so by doing some more search .

I arrive at Recoder req.end ( ~ line 349 ), with

chunk = null
encoding = null
callback = undefined

so, doesn't call recordChunk ( ~ line 360 ), because !chunk .
And calling :
oldEnd.call(req, chunk, encoding, callback)

So, I arrive in intercepted_request_router.js req.end (~ line 142), with :

chunk = null
encoding = null
callback = undefined

and call req.write at ~ line 154 .

So finally I arrive at Recoder req.write ( ~ line 336 ), with

chunk = null
encoding = null

And so calling recordChunk with chunk null ....

So, It seems, write is not called before end, but end call write ... But I'm not an expert of node HTTP client, I always use a library to do this ...

I'll continue my research, just, is it normal that end call write with null ? ( end check if chunk is not null to call recordChunk, but pass null to write, and write doesn't check if recordChunk is null )

---

Edit :

It seems, in most of the case, oldEnd is the function here https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L745 ,
but sometimes, it's

(...args) => this.handleEnd(...args)

from intercepted_request_router.js (~ line 77 )

---

Edit 2 :
Oh .... It seems I've some parrallel requests too .... will be funny ...

[paulmelnikow]

Hi! I appreciate your digging into this. I wanted to convey that in order to merge a patch for a problem like this, we will need a reproducible test example. That's for two reasons: (1) we need to include a test case that's invoked either via nock's public API or via an http request which covers the issue, and (2) we need to make sure we're maintaining parity with Node's behavior, not implementing workarounds for bugs in client libraries.

[thib3113]

yes yes I totally understand ... I'm just trying to search how this bug appear (and if it's really a bug) :/ ...

So for the moment, I didn't have any idea how to produce a reproductible this bug each time ...

But, if you have any idea, about things to check/search, I'm interested

[thib3113]

Hum, sorry, I'm always trying to find the error ...

Is it possible that nock record and try to mock at the same time ?

Just to be sure, to record, I use :

//start
nock.recorder.clear();
nock.recorder.rec({
                dont_print: true,
                output_objects: true,
                enable_reqheaders_recording: true
            });

//do some requests

//stop record
nock.recorder.play() // to store content
//loop on each 302 response
nock.recorder.play() // to store the response from 302

nock.restore();
nock.activate();
nock.enableNetConnect();

and to play I use :

nock.enableNetConnect(host => host.includes('localhost') || host.includes('127.0.0.1') || host.includes('mongo'));
nock.define(nock.loadDefs(file)); // sometimes adding a loop to persist some calls
nock.recorder.play();

//do some requests

nock.restore();
nock.activate();

( when doing some request, I use nock for some specific things sometimes, so I need to reactivate before )

[thib3113]

Hello,
I've done some more tests, and I see this :

I've add breakpoints on this lines :

  http.ClientRequest = OverriddenClientRequest // line 318

and

debug('restoring overridden ClientRequest') // 324

so it breaks at line 318
you store the function "ClientRequest", in originalClientRequest

And it breaks on line 324 :
originalClientRequest is undefined

( so you don't restore http.ClientRequest to originalClientRequest )

next time it breaks at line 318

http.ClientRequest is now a OverriddenClientRequest ( and not a ClientRequest ) .

This seems strange for me ... but maybe normal ?
and this three breakpoints was called before I do something ... Coming from back.js line 113 and 115 ...

( I can reproduce this strange process if you want )
( here is a repo reproducing this : https://github.com/thib3113/test-nock , just start yarn test , tests doesn't pass, but the strange process is done before testing )

[mastermatt]

Breakpoints pause execution at the beginning of a line, so the line has yet to execute. originalClientRequest may be undefined on 324, but if you step one line it should be set.

[thib3113]

@mastermatt
here is line 324 :
debug('restoring overridden ClientRequest') // 324
it's not setting originalClientRequest .

And below, debugger pass in if (!originalClientRequest) {
and so log this line : https://github.com/nock/nock/blob/master/lib/intercept.js#L328

So, originalClientRequest seems undefined

Maybe more clear with debug logs ?

  nock.recorder 0 restoring all the overridden http/https properties +0ms
  nock.intercept restoring overridden ClientRequest +0ms
  nock.intercept - ClientRequest was not overridden +0ms
  nock.intercept Overriding ClientRequest +635ms
  nock.intercept ClientRequest overridden +629ms

//some of my code run here

  nock.recorder 0 restoring all the overridden http/https properties +0ms
  nock.intercept restoring overridden ClientRequest +0ms
  nock.intercept - ClientRequest was not overridden +865ms

Has you can see, debug say :

  nock.intercept Overriding ClientRequest +635ms

( +635ms come from my breakpoint )
and just after

  nock.intercept restoring overridden ClientRequest +0ms
  nock.intercept - ClientRequest was not overridden +865ms

( +865ms is my breakpoint )

---

It seems, this strange behavior, come because I import nock two times, and one in jest setup .
Example with my test repo :

• Recorder.ts import nock : https://github.com/thib3113/test-nock/blob/master/tests/scripts/Recorder.ts#L4
• setup.ts ( globalSetup in jest.config ) https://github.com/thib3113/test-nock/blob/master/tests/scripts/setup.ts#L4 import Recorder (and seems to override http.ClientRequest first time)
• test.test.ts (my fake test) https://github.com/thib3113/test-nock/blob/master/tests/test.test.ts#L4 import Recorder too (override http.ClientRequest second time, but originalClientRequest is undefined ...)

Maybe linked with the way jest manage setup ? and tests ?

[thib3113]

So, It seems that I find a solution to resolve this problem .

Here is a comparaison of my fix : master...thib3113:master

I'm not sure it's perfect, but it seems to work for my project . ( just, it seems your tests doesn't work on windows, so I'll need to build a linux Vm to start the tests )

---

Edit :
All the tests pass correctly .

So, no idea about how to write a test for it ( write a test, that start some jest tests ? ) ( I doesn't know if you need to support jest, or if jest need to support you, but in my opinion a mocking library need to test compatibility with test frameworks ? )

[mastermatt]

Ahh I think I missed where you mentioned you are running Jest.
See discussion here #1817

[thib3113]

Hum, yes seems to be linked ...

Ok, so if I understand correctly, jest reset import between each tests, but can't reset the "native imports" (like http here) (and it seems nodejs doesn't support the clear from cache for "native libraries") ...

So, what can be the solution ?
Because, my solution will grow this heap memory (because instead of storing old createRequest, I will store each old createRequest) ...

If I correctly understand, you can't really fix this ? (or checking if you already update the native library, and skip the update if already done) .

So, the solutions I see, are :

• stop using jest (research for another test framework, need migrations, and maybe lot of others problems)
• stop using nock (seems not easy too)
• fork nock with my update ( it's not a really good option, because I'll need to follow each update from official nock ... But, will be the faster answer ... and my boss already dislike I spend two days to debug my problem )

[mastermatt]

Is the suggested method of calling nock.restore() after each test suite not solving your issue?

[thib3113]

Hum, I'm testing ... ( In the time I wrote my message, all tests seems to run correctly )

But, in fact, because I import nock in globalSetup, I need to restore it in the global setup .
( so, I think it will be the same for testEnvironment, setupFilesAfterEnv, setupFiles .... ) .

Maybe an update in the readme about this, can be interessting ?