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

Reading commit with node 22.3.0 #1933

Open
matteo-cristino opened this issue Jun 17, 2024 · 25 comments · May be fixed by #1939
Open

Reading commit with node 22.3.0 #1933

matteo-cristino opened this issue Jun 17, 2024 · 25 comments · May be fixed by #1939

Comments

@matteo-cristino
Copy link

Using node with version 22.2.0 all works perfectly, but using version 22.3.0 (that is the latest) I got the following error when trying to read a commit:

TypeError {
  message: 'Cannot create property \'caller\' on string \'incorrect header check\'',
}

TypeError: Cannot create property 'caller' on string 'incorrect header check'
    at Object.readCommit (/home/matteo/src/slangroom/node_modules/.pnpm/isomorphic-git@1.25.10/node_modules/isomorphic-git/index.cjs:12665:16)
@jcubic
Copy link
Contributor

jcubic commented Jun 17, 2024

So this project doesn't have an original author that left the project. So right now the status is that's community driven. Which means that if you will not debug this yourself, there will need to be someone that will. Are you this person?

@matteo-cristino
Copy link
Author

I can have a look, but I can not find anywhere the documentation saying how can I launch tests. I tried pnpm i followed by pnpm test but it breaks, am I doing something wrong?

@matteo-cristino
Copy link
Author

I managed to have the tests working on node 20.11.1 looking at #1902, but I got an error on gitlab tests

  • Hosting Providers › GitLab › push
  • Hosting Providers › GitLab › fetch
HttpError: HTTP Error: 401 Unauthorized

For what I can understand the token has expired. I also tried to clone that repo from command line with

git clone https://isomorphic-git-test-push:<token>@gitlab.com/isomorphic-git/test.empty

(do not want to write directly the token here, but it is the one from test file) and I got

Cloning into 'test.empty'...
remote: HTTP Basic: Access denied. The provided password or token is incorrect or your account has 2FA enabled and you must use a personal access token instead of a password. See https://gitlab.com/help/topics/git/troubleshooting_git#error-on-git-fetch-http-basic-access-denied
fatal: Authentication failed for 'https://gitlab.com/isomorphic-git/test.empty/'

@jcubic
Copy link
Contributor

jcubic commented Jun 18, 2024

Yes, those tests are failing, I need to contact the original author to get access to that repo on GitLab.

@mojavelinux
Copy link
Contributor

mojavelinux commented Jun 23, 2024

I'm seeing this issue too. I'm getting it when using statusMatrix, but I have no idea where the error is actually coming from. I've yet to be able to create a reproducer in isolation.

UPDATE: Obviously, the reported error is when it is trying to set the caller property on a string. What I can't figure out is how that string error is being thrown, which is the real underlying issue.

@mojavelinux
Copy link
Contributor

mojavelinux commented Jun 23, 2024

Here's the actual source of the error:

     Error: incorrect header check
      at Object.inflate (node_modules/pako/lib/inflate.js:389:29)
      at inflate (node_modules/isomorphic-git/index.cjs:2669:12)
      at _readObject (node_modules/isomorphic-git/index.cjs:3156:39)

It appears that something changed in Node.js that is breaking pako's inflate function.

UPDATE: The problem seems to be with the creation of repositories, not reading them. After isomorphic-git creates the repository, even canonical git reports "incorrect header check". So the issue may not be with the inflate function but rather elsewhere.

@mojavelinux
Copy link
Contributor

If we switch from pako to the zlib.deflate provided by Node.js, it works. So there seems to be a problem with pako's deflate method when running on Node.js 22.3.0.

I wish isomorphic-git would use zlib from Node.js when running in Node. Perhaps that's one way out of this.

@mojavelinux
Copy link
Contributor

Oh, I might see what the problem is. Node.js has implemented the CompressionStream class from the web. So now it gets selected (instead of pako). But it's not doing the same work as what isomorphic-git expects and is thus creating a corrupt zlib-compressed object. If I comment out that check and force it to use pako, then it works.

@mojavelinux
Copy link
Contributor

What we probably need is a new release of isomorphic-git that updates the following method to force Node.js to use pako.

function testCompressionStream() {
  try {
    const cs = new CompressionStream('deflate');
    cs.writable.close();
    // Test if `Blob.stream` is present. React Native does not have the `stream` method
    const stream = new Blob([]).stream();
    stream.cancel();
    return true
  } catch (_) {
    return false
  }
}

@mojavelinux
Copy link
Contributor

Actually, isomorphic-git uses Node.js' CompressionStream class when using Node.js 22.2.0. For some reason, using it under 22.3.0 causes it to create a corrupt result. So perhaps there is a bug in Node.js here.

Still, I think Node.js should use pako or even zlib instead of CompressionStream.

@jcubic
Copy link
Contributor

jcubic commented Jun 23, 2024

BTW: I disabled the GitLab tests, so now the tests should work fine. I also got contact from the original author, he gave me creds for GitLab, but I haven't checked them yet.

@mojavelinux
Copy link
Contributor

Note that the reason this isn't a problem with inflate is because isomorphic-git always uses pako in that case. See https://github.com/isomorphic-git/isomorphic-git/blob/main/src/utils/inflate.js#L5

@mojavelinux
Copy link
Contributor

mojavelinux commented Jun 23, 2024

I'm rather confident this is a bug in Node.js 22.3. Consider the following code:

;(async () => {
  console.log(await deflate('deflate me'))
})()

async function deflate(buffer) {
  const cs = new CompressionStream('deflate')
  const c = new Blob([buffer]).stream().pipeThrough(cs)
  return new Uint8Array(await new Response(c).arrayBuffer())
}

We expect the following result:

Uint8Array(18) [
  120, 156, 75,  73, 77, 203, 73,
   44,  73, 85, 200, 77,   5,  0,
   21,  96,  3, 200
]

We get the same result with both pako.deflate and zlib.deflate too.

However, with Node.js 22.3.0, we get this instead:

Uint8Array(8192) [
   47,   0,   0,   0,   0,   0,   0,   0,  13,  10,   0,   0,
    0,   0,   0,   0,  72,  84,  84,  80,  47,  49,  46,  49, ...
]

So something is massively wrong.

@mojavelinux
Copy link
Contributor

I discovered a workaround that works on Node.js 22.2.0 and 22.3.0 (and probably all other Node.js versions too).

async function browserDeflate(buffer) {
  const cs = new CompressionStream('deflate');
  const c = new Blob([buffer]).stream().pipeThrough(cs);
  return new Response(c).blob().then((b) => new Response(b).arrayBuffer()).then((ab) => new Uint8Array(ab))
}

Notice that we are retrieving the blob, running it through another Response object, then converting it to Uint8Array. I have no idea why this works, but it seems like the arrayBuffer returned from the Response created with the original blob stream doesn't work in Node.js 22.3.0.

@jcubic
Copy link
Contributor

jcubic commented Jun 23, 2024

There was a similar issue recently:

When Google Chrome changed the algorithm of CompressionStream, maybe Node did the same.

@verreauxblack
Copy link

I'm also seeing the same issue when I use push method.
TypeError: Cannot create property 'caller' on string 'incorrect header check.

But I think error occured when I used add method.

$ git status
error: inflate: data stream error (incorrect header check)
error: unable to unpack 0ce8c6a6fd82131a7f89d52a80ecb4b9112271bb header
fatal: loose object 0ce8c6a6fd82131a7f89d52a80ecb4b9112271bb (stored in .git/objects/0c/e8c6a6fd82131a7f89d52a80ecb4b9112271bb) is corrupt

@mojavelinux
Copy link
Contributor

Yes, I believe that is correct. It is writing garbage files to the .git/objects directory, which it subsequently fails to read.

@mojavelinux
Copy link
Contributor

mojavelinux commented Jun 24, 2024

I've submitted a PR.

The reason the tests pass even when using Node.js 22.3.0 is because the global variables for compression streams were not being seen in the test environment. I've modified the Jest configuration so that they are visible, which then triggers the reported error.

@matteo-cristino
Copy link
Author

Thank you very much for your quick and great help! ✨

I was planning to start debugging it this week that I have more free time, anyway if I can be helpfull in some ways leave a message and I will look into it.

@mojavelinux
Copy link
Contributor

I'd still like to know why the original code doesn't work with Node.js 22.3.0. Was the code wrong and it just happened to work, or is this really a bug in Node.js 22.3.0? The workaround is portable, but it might be suppressing an underlying issue. It would also be nice to avoid the double use of Response.

@jcubic
Copy link
Contributor

jcubic commented Jun 24, 2024

I would start a discussion in the Node repo.

@mojavelinux
Copy link
Contributor

I'm not willing to go down that road, but someone else is welcome to.

@matteo-cristino
Copy link
Author

I opened a discussion over there https://github.com/orgs/nodejs/discussions/53581 copying your test code that explained the issue perfectly, will let you known if anyone answer to me

@matteo-cristino
Copy link
Author

They answer, it is a node issue and they are aware of it. A fix is coming in v22.4.0 in a couple of days.

Do you want me to ask further if there is a way to be compatible with all node 22 versions or are you good with removing 22.3.0 as supported version from the engines section in the package.json?

"engines": {
"node": ">=12"
},

@jcubic
Copy link
Contributor

jcubic commented Jun 25, 2024

I think that removing 22.3.0 is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants