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

Node v9.x.x compatibility #95

Closed
lmm-git opened this Issue Nov 22, 2017 · 7 comments

Comments

Projects
None yet
7 participants
@lmm-git

lmm-git commented Nov 22, 2017

This module seems not to be compatible with Node versions >= 9.

Executing npm run test with node version 9.2.0 results in various errors:

(Part of npm run test)

root@b80709335c29:/pngjs# node_modules/tape/bin/tape test/convert-images-spec.js 
Converting images
TAP version 13
# convert sync - PngSuite.png
/usr/local/bin/node[730]: ../src/node_zlib.cc:189:static void node::{anonymous}::ZCtx::Write(const v8::FunctionCallbackInfo<v8::Value>&) [with bool async = false]: Assertion `Buffer::HasInstance(args[4])' failed.
 1: node::Abort() [node]
 2: node::Assert(char const* const (*) [4]) [node]
 3: 0x1258860 [node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 5: 0xb77f9c [node]
 6: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 7: 0x15e5b77042fd
Aborted (core dumped)

For me they seem to be related to nodejs/node#13322 and therefore with this commit: nodejs/node@add4b0a

There the interface used in lib/sync-inflate.js changes, therefore the first line failing is line 106 (on current master).

var res = this._handle.writeSync(flushFlag,

By changing line 110 and 111 to use the new internal variable names it is at least possible to circumvent the program exception (by breaking backward compatibility). But then there are several other errors which are not as easy to fix (as far as I have seen).

I'm not sure how to fix this issue by maintaining backward compatibility because there does not seem to exists an "easy" fix to support the new interface. I'm also not sure whether it is necessary to work with the internal state of zlib and not using the official API (which did not change I believe).

Probably someone of the project maintainers could take a look into it or could just shortly write how they plan to make the module ready for Node 9. Probably I am just overlooking something.

@anescobar1991

This comment has been minimized.

anescobar1991 commented Dec 28, 2017

@lukeapage could you or another maintainer advice on this one?

@LinusU

This comment has been minimized.

LinusU commented Jan 27, 2018

I wish I had searched the issues before trying to track down this myself 😂

Just arrived at the same line, the call to writeSync and saw that _buffer and _offset was undefined. As you say, just changing the names gives a ton of other errors, but prevents the crash.

I experimented with trying to use the builtin inflateSync instead, and it passes for most images. For a number of them though, the chunkSize gets set to a value that seems to be too low for zlib to accept.

Removing the custom chunkSize seems to trigger another timeout when reading in the ChunkStream class. Currently looking into if there is any way to fix that...

update:

Interestingly, the following patch works fine on Node.js 6.x. So it seems like my problem in ChunkStream didn't have anything to do with these changes, but rather is yet another problem in 9.x that we need to solve...

diff --git a/lib/parser-sync.js b/lib/parser-sync.js
index a49e386..342becf 100644
--- a/lib/parser-sync.js
+++ b/lib/parser-sync.js
@@ -67,14 +67,7 @@ module.exports = function(buffer, options) {
   var inflateData = Buffer.concat(inflateDataList);
   inflateDataList.length = 0;
 
-  var inflatedData;
-  if (metaData.interlace) {
-    inflatedData = zlib.inflateSync(inflateData);
-  } else {
-    var rowSize = ((metaData.width * metaData.bpp * metaData.depth + 7) >> 3) + 1;
-    var imageSize = rowSize * metaData.height;
-    inflatedData = inflateSync(inflateData, { chunkSize: imageSize, maxLength: imageSize });
-  }
+  var inflatedData = zlib.inflateSync(inflateData);
   inflateData = null;
 
   if (!inflatedData || !inflatedData.length) {
@threehams

This comment has been minimized.

threehams commented Mar 1, 2018

A PR was merged for this, so it's just waiting on a release.
#102

@lukeapage

This comment has been minimized.

Owner

lukeapage commented Mar 2, 2018

Actually the Pr is published when it was merged as 3.3.2...

@lukeapage lukeapage closed this Mar 2, 2018

@threehams

This comment has been minimized.

threehams commented Mar 2, 2018

Got it! I was looking at the Github releases page, not npm.

threehams added a commit to threehams/jest-image-snapshot that referenced this issue Mar 2, 2018

Update pngjs for Node 9 support.
pngjs now works with Node 9. This updates the minimum dependency.

lukeapage/pngjs#95 (comment)
lukeapage/pngjs#102
@akabekobeko

This comment has been minimized.

akabekobeko commented Mar 19, 2018

I tried updating to pngjs v3.3.2, but the problem has not been fixed. The same error occurs when used in Node v9.8.0 and macOS v10.13.3 environment.

Clone this repositiory.

$ git clone https://github.com/lukeapage/pngjs.git
$ cd pngjs
$ npm i

Rewrite the test command of package.json to make it easier to understand.

{
  "scripts": {
    "test": "tape test/*-spec.js"
  }
}

Run the test.

$ npm test
...
ok 75 should be equal
# should bail with an error given an invalid PNG
ok 76 Error should be received
# should return an error if a PNG is normal except for a missing IEND
ok 77 Error should be received
# convert sync - PngSuite.png
node[3089]: ../src/node_zlib.cc:188:static void node::(anonymous namespace)::ZCtx::Write(const FunctionCallbackInfo<v8::Value> &) [async = false]: Assertion `Buffer::HasInstance(args[4])' failed.
 1: node::Abort() [/usr/local/bin/node]
 2: node::Assert(char const* const (*) [4]) [/usr/local/bin/node]
 3: void node::(anonymous namespace)::ZCtx::Write<false>(v8::FunctionCallbackInfo<v8::Value> const&) [/usr/local/bin/node]
 4: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/usr/local/bin/node]
 5: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/usr/local/bin/node]
 6: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/usr/local/bin/node]
 7: 0xc67bc6042fd
 8: 0xc67bc6bd196
 9: 0xc67bc60535f
@yanivefraim

This comment has been minimized.

yanivefraim commented Mar 20, 2018

@lukeapage - The issue was not fixed in v3.3.2, can you plz take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment