Skip to content

fix: hasBinary flakiness #16641

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 26, 2021
Merged

fix: hasBinary flakiness #16641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions packages/socket/patches/socket.io-parser+4.0.4.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ index 0ef9f80..4cd787e 100644
catch (e) {
return false;
diff --git a/node_modules/socket.io-parser/dist/is-binary.js b/node_modules/socket.io-parser/dist/is-binary.js
index 4b7c234..95469f7 100644
index 4b7c234..15ed5b1 100644
--- a/node_modules/socket.io-parser/dist/is-binary.js
+++ b/node_modules/socket.io-parser/dist/is-binary.js
@@ -1,6 +1,7 @@
Expand All @@ -126,7 +126,7 @@ index 4b7c234..95469f7 100644
const withNativeArrayBuffer = typeof ArrayBuffer === "function";
const isView = (obj) => {
return typeof ArrayBuffer.isView === "function"
@@ -22,13 +23,18 @@ const withNativeFile = typeof File === "function" ||
@@ -22,16 +23,21 @@ const withNativeFile = typeof File === "function" ||
function isBinary(obj) {
return ((withNativeArrayBuffer && (obj instanceof ArrayBuffer || isView(obj))) ||
(withNativeBlob && obj instanceof Blob) ||
Expand All @@ -146,7 +146,11 @@ index 4b7c234..95469f7 100644
+ known.push(obj)
if (Array.isArray(obj)) {
for (let i = 0, l = obj.length; i < l; i++) {
if (hasBinary(obj[i])) {
- if (hasBinary(obj[i])) {
+ if (hasBinary(obj[i], known)) {
return true;
}
}
Comment on lines -149 to +153
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cause of the problem was the incorrect recursive call of hasBinary inside check Array section. Without known argument being passed, when checking arrays that have circular references, hasBinary always starts from fresh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 x 10000000

@@ -43,10 +49,10 @@ function hasBinary(obj, toJSON) {
if (obj.toJSON &&
Copy link

@amakhrov amakhrov Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random observation: this if condition is always false, because it assumes the hasBinary function can be called with only one argument. It was possible in the original code (optional toJson argument) , but not possible with the patched version, where you always pass known as the second arg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.

The full code is like below:

function hasBinary(obj, known = []) {
    if (!obj || typeof obj !== "object") {
        return false;
    }
    if (known.includes(obj)) {
        return false
    }
    known.push(obj)
    if (Array.isArray(obj)) {
        for (let i = 0, l = obj.length; i < l; i++) {
            if (hasBinary(obj[i], known)) {
                return true;
            }
        }
        return false;
    }
    if (isBinary(obj)) {
        return true;
    }
    if (obj.toJSON &&
        typeof obj.toJSON === "function" &&
        arguments.length === 1) {
        return hasBinary(obj.toJSON(), known);
    }
    for (const key in obj) {
        if (Object.prototype.hasOwnProperty.call(obj, key) && hasBinary(obj[key], known)) {
            return true;
        }
    }
    return false;
}

When we pass [1, {}] as obj. hasBinary(1, [[1, {}]]) returns false but hasBinary({}, [[1, {}], 1]) returns true.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I probably was not very clear. I specifically meant the if condition that handles a very special use case, when an object has toJSON method to be called.
This if condition checks that aruments.length === 1 - which is never the case in the patched version (both your examples have arguments.length === 2 because of the known argument).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Forget the old comment. I misunderstood your comment again.

I believe this can be fixed by removing arguments.length === 1. Maybe I need to create a new test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amakhrov I opened the issue at #17988.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

typeof obj.toJSON === "function" &&
Expand Down
35 changes: 35 additions & 0 deletions packages/socket/test/socket_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,41 @@ describe('Socket', function () {
}
})

it('correctly encodes and decodes circular data in array', (done) => {
const encoder = new parser.Encoder()

const circularObj = {
foo: {},
}

circularObj.foo.circularArray = [circularObj, circularObj]

const obj = {
type: PacketType.EVENT,
data: ['a', circularObj],
id: 23,
nsp: '/cool',
}

const originalData = obj.data

const encodedPackets = encoder.encode(obj)

const decoder = new parser.Decoder()

decoder.on('decoded', (packet) => {
obj.data = originalData
expect(packet.data[1] === packet.data[1].foo.circularArray[0]).to.be.true
expect(packet.data[1] === packet.data[1].foo.circularArray[1]).to.be.true
expect(packet).to.eql(obj)
done()
})

for (let i = 0; i < encodedPackets.length; i++) {
decoder.add(encodedPackets[i])
}
})

it('correctly encodes and decodes circular data containing binary', (done) => {
const encoder = new parser.Encoder()

Expand Down