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

fix: make port transferables unique #3421

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

icidasset
Copy link
Contributor

Fixes #3402

The transfer list, ie. the second argument to postMessage, should always contain unique values, multiple of the same pointers cause errors. This PR fixes that, see linked issue above for more info. My solution was to just make everything in the list unique by converting the array to a set and back. Let me know if this works, or if another solution is preferred.

@welcome
Copy link

welcome bot commented Nov 25, 2020

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@achingbrain
Copy link
Member

This looks great, thanks for submitting it. Please can you add a test to ensure regressions won't occur in future?

@icidasset icidasset force-pushed the unique-transferables branch from 6693663 to 1721825 Compare November 27, 2020 14:36
@icidasset
Copy link
Contributor Author

icidasset commented Nov 27, 2020

@achingbrain I added a test for ipfs-message-port-server.
Can confirm that the test fails when I remove the code changes.

When the test is ran in the browser environment, it does complain about worker_threads not being available, even though I'm detecting the correct environment. Possibly webpack that tries to resolve the require before the code is actually being executed? 🤷 Anyhow, it does seem to actually run the tests.

@achingbrain achingbrain requested a review from Gozala November 30, 2020 18:22
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.

Thanks @icidasset for identifying this issue and also fixing it! It looks good. There would be few nice to haves, but I would much rather land this now and address them in followups. Feel free to take a stab if you like, otherwise I'll do it when I'll get around to it.

  1. Implement solution does more or less following [...new Set(...transfer)] to eliminate duplicates. But ideally we would just make transfer itself into a Set so we don't have to eliminated duplicates.
  2. postMessage actually accepts Sets (or any iterable) for transfer list, so conversion back to array is unnecessary.
  3. I think it would probably be better to amend ipfs-message-port-protocol tests to exercise moving nodes e.g with duplicates CIDs.

@Gozala Gozala merged commit da7bc55 into ipfs:master Dec 1, 2020
@achingbrain
Copy link
Member

I would much rather land this now and address them in followups.

@Gozala please can you open issues for the outstanding pieces of work so they don't get missed

@Gozala
Copy link
Contributor

Gozala commented Dec 1, 2020

@Gozala please can you open issues for the outstanding pieces of work so they don't get missed

#3402 (comment)

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.

ipfs-message-port-* should ensure no transferables are duplicated
3 participants