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

Fix encoder instanceof issue with Encoder #685

Merged
merged 4 commits into from
Apr 29, 2020
Merged

Conversation

Nevon
Copy link
Collaborator

@Nevon Nevon commented Apr 2, 2020

@bfdill was having this issue where KafkaJS was failing to encode requests because Encoder.writeEncoder was throwing, because the value was not an instanceof Encoder, even though we could clearly see that it was in fact an encoder. I don't have an explanation for this, but we have seen issues with instanceof before. I'm guessing that his project was doing something strange at build time, since it was a Typescript project (maybe he's accidentally transpiling node_modules).

Error: value should be an instance of Encoder
        at Encoder.writeEncoder (/Users/user/projects/project/node_modules/kafkajs/src/protocol/encoder.js:177:13)
        at Object.<anonymous>.module.exports (/Users/user/projects/project/node_modules/kafkajs/src/protocol/request.js:10:6)
        at sendRequest (/Users/user/projects/project/node_modules/kafkajs/src/network/connection.js:276:30)
        at Connection.send (/Users/user/projects/project/node_modules/kafkajs/src/network/connection.js:303:53)
        at Broker.apiVersions (/Users/user/projects/project/node_modules/kafkajs/src/broker/index.js:153:20)
        at Broker.connect (/Users/user/projects/project/node_modules/kafkajs/src/broker/index.js:93:25)
        at /Users/user/projects/project/node_modules/kafkajs/src/cluster/brokerPool.js:79:9
        at Cluster.connect (/Users/user/projects/project/node_modules/kafkajs/src/cluster/index.js:104:5)
        at Object.connect (/Users/user/projects/project/node_modules/kafkajs/src/producer/index.js:205:7) {
      [stack]: 'Error: value should be an instance of Encoder\n' +
        '    at Encoder.writeEncoder (/Users/user/projects/project/node_modules/kafkajs/src/protocol/encoder.js:177:13)\n' +
        '    at Object.<anonymous>.module.exports (/Users/user/projects/project/node_modules/kafkajs/src/protocol/request.js:10:6)\n' +
        '    at sendRequest (/Users/user/projects/project/node_modules/kafkajs/src/network/connection.js:276:30)\n' +
        '    at Connection.send (/Users/user/projects/project/node_modules/kafkajs/src/network/connection.js:303:53)\n' +
        '    at Broker.apiVersions (/Users/user/projects/project/node_modules/kafkajs/src/broker/index.js:153:20)\n' +
        '    at Broker.connect (/Users/user/projects/project/node_modules/kafkajs/src/broker/index.js:93:25)\n' +
        '    at /Users/user/projects/project/node_modules/kafkajs/src/cluster/brokerPool.js:79:9\n' +
        '    at Cluster.connect (/Users/user/projects/project/node_modules/kafkajs/src/cluster/index.js:104:5)\n' +
        '    at Object.connect (/Users/user/projects/project/node_modules/kafkajs/src/producer/index.js:205:7)',
      [message]: 'value should be an instance of Encoder'
    }

I thought about adding a name property to every instance of Encoder, but it feels a bit ugly, so I decided to go for an approach where we instead just check that the value is "Encoder-like", i.e. has a buffer property with a Buffer inside it, since that's what we actually care about. Anyway the encoder is only an internal class, so it's mostly there to prevent us from shooting ourselves in the foot.

This is to ensure the instanceof check works with preprocessing like uglify or minify, which may change `value.constructor.name`.
…face

Rather than checkingt that the provided value is an actual encoder, we just
check if it has the required property of the right type. That way we avoid issues
where instanceof doesn't work, while being able to avoid public name properties and
string comparisons.
@Nevon Nevon requested review from tulios and JaapRood April 2, 2020 16:32
@bfdill
Copy link

bfdill commented Apr 2, 2020

Based on a little bit of additional testing, I'm wondering if this stems from a bug in require?

nodejs/node#13408

Thanks for the help working through this today!

@Nevon
Copy link
Collaborator Author

Nevon commented Apr 2, 2020

That wouldn't explain why you are the only person that has this problem, though. This should affect literally everyone, since it happens on the very first request we make.

@bfdill
Copy link

bfdill commented Apr 2, 2020

That's true, but it does explain the behavior I'm seeing. What I'm not sure about is how my setup is forcing this issue. We are in a monorepo, that project is using typescript, the test is running via ts-node.

When I finish the project I'm on right now, I'll try to see if I can get a minimal reproduction with the current version.

@tulios tulios removed the request for review from JaapRood April 28, 2020 23:12
@Nevon Nevon merged commit cfbb49d into master Apr 29, 2020
@Nevon Nevon deleted the fix-encoder-instanceof-issue branch April 29, 2020 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants