Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

chore: Replace transfer arrays with sets in ipfs-message-port-* #3481

Closed
wants to merge 2 commits into from

Conversation

icidasset
Copy link
Contributor

@icidasset icidasset commented Jan 14, 2021

  1. Fixes Arraybuffer at index 0 is already detached #3480
    As described in the issue, I wasn't sure how to tackle this, but I decided to go the "remove already detached buffers" route. This seems to work, I tested this with my app (see issue for more info). Thoughts?

  2. Instead of passing of an Array to postMessage, we can just pass the Set iterable.
    The only downside of this seems to be that Typescript doesn't like this 🤔
    Typescript expects an Array, not an iterable like it should.

  3. I just discovered there's a thing called SharedArrayBuffer, maybe we should use that instead? 🤔
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer

@Gozala
Copy link
Contributor

Gozala commented Jan 15, 2021

Thanks @icidasset for tackling the problem. I'll respond to some of your comments inline below:

  • Fixes Arraybuffer at index 0 is already detached #3480
    As described in the issue, I wasn't sure how to tackle this, but I decided to go the "remove already detached buffers" route. This seems to work, I tested this with my app (see issue for more info). Thoughts?

I would really like this fixed, but I can't shake the feeling that proposed changes

  1. Work around the issue without actually fixing it.
  2. Actual issue is still there it's just fission happens to not detect it yet.
  3. Dropped buffer would lead on the receiver end getting Uint8Array copy with size 0 as opposed to expected bytes. And that Uint8Array is most likely was not intended to be 0 size.

I really think we should at least figure out what causes this issue. I'll provide some specific suggestions on this in the #3480

  • Instead of passing of an Array to postMessage, we can just pass the Set iterable.
    The only downside of this seems to be that Typescript doesn't like this 🤔
    Typescript expects an Array, not an iterable like it should.

@ts-ignore is unfortunate, but at this point we're used to typescript standard lib not been perfect.

SharedArrayBuffers had been disabled in response to Spectre vulnerability, I think chrome enabled them back but it's unclear when others will do it.
But even if we had them available, they aren't necessarily a good fit here, they create a shared memory and require synchronized access. Transferring buffers seems like a lot better fit when one thread writes a chunk of memory and then passes it onto the other one to read/write into it.

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

@icidasset I'm happy to take changes that switch from arrays to sets, however we need to figure out proper fix for #3480 as dropping such array buffers do not really fix the problem they'll just push it further down the stack.

packages/ipfs-message-port-server/src/server.js Outdated Show resolved Hide resolved
packages/ipfs-message-port-server/src/server.js Outdated Show resolved Hide resolved
packages/ipfs-message-port-protocol/src/core.js Outdated Show resolved Hide resolved
@icidasset icidasset changed the title Fixes #3480 Chore: Replace transfer arrays with sets in ipfs-message-port-* Jan 21, 2021
@icidasset
Copy link
Contributor Author

@Gozala While we discuss the mysterious buffer transfer issues in #3480, I repurposed this PR to just replace the arrays with sets. I also applied the changes you asked for.

@icidasset icidasset changed the title Chore: Replace transfer arrays with sets in ipfs-message-port-* chore: Replace transfer arrays with sets in ipfs-message-port-* Jan 21, 2021
@icidasset icidasset requested a review from Gozala January 27, 2021 17:20
@icidasset
Copy link
Contributor Author

@Gozala Do we still want this tiny PR that sends a Set over postMessage instead of [...set]?

@Gozala Gozala mentioned this pull request Feb 26, 2021
@Gozala
Copy link
Contributor

Gozala commented Feb 27, 2021

@Gozala Do we still want this tiny PR that sends a Set over postMessage instead of [...set]?

@icidasset thanks for starting this effort, I have taken on your work to create #3573 which removes transferable arrays everywhere and just makes them sets. I would push changes here, but github won't allow because PR is from your fork. So I'll close this in favor of the other one.

@Gozala Gozala closed this Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arraybuffer at index 0 is already detached
2 participants