Skip to content

feat: Configure maxDataLength for it-length-prefixed in PeerStreams #2954

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

Merged
merged 14 commits into from
Feb 18, 2025

Conversation

acul71
Copy link
Contributor

@acul71 acul71 commented Feb 11, 2025

feat: Allow configuration of it-length-prefixed maxDataLength via PeerStreams Options

Description

This PR introduces a new feature that allows users to configure the maxDataLength option used by it-length-prefixed through the PeerStreams options. Previously, the library defaulted to a maximum data length of 4MB, which could lead to issues when processing larger messages. With this change, both the encoder and decoder in it-length-prefixed can be configured to use a custom maxDataLength (e.g., 8MB), thus accommodating larger messages.

In addition, I have upgraded it-length-prefixed to version 10.0.0 in packages/pubsub/package.json. However, I'm not sure if this is the only place where the upgrade is required. A review of other packages that depend on it-length-prefixed might be necessary.

A new test has been added to validate the behavior, ensuring that a 6MB message is correctly transmitted and received without triggering an error, and that the content of the sent and received messages are identical.

Notes & open questions

  • The upgrade to [email protected] in packages/pubsub/package.json has been performed, but please verify if additional changes are needed in other parts of the repository.
  • The new configuration option is applied to both encoding and decoding. Feedback on the approach or any potential backward compatibility issues would be appreciated.
  • Are there any additional tests or documentation updates required for this feature?

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@acul71 acul71 requested a review from a team as a code owner February 11, 2025 13:15
@acul71 acul71 changed the title feat: Allow Configuration of it-length-prefixed maxDataLength via peerStreams Options feat: Configure maxDataLength for it-length-prefixed in PeerStreams Feb 11, 2025
@dhuseby
Copy link
Contributor

dhuseby commented Feb 11, 2025

answer: modify it everywhere

@acul71
Copy link
Contributor Author

acul71 commented Feb 11, 2025

@achingbrain @SgtPooki let me know

@SgtPooki
Copy link
Member

Is the interop CI failure due to these changes or do we have a flaky test?

@acul71
Copy link
Contributor Author

acul71 commented Feb 13, 2025

@SgtPooki @achingbrain @dhuseby Update it-length-prefixed dependency version in all packages to ^10.0.0.

@acul71 acul71 closed this Feb 13, 2025
@acul71 acul71 reopened this Feb 13, 2025
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, small nit about the test inline.

Copy link
Contributor Author

@acul71 acul71 left a comment

Choose a reason for hiding this comment

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

Ok if I understand this correctly the

import type { DecoderOptions as LpDecoderOptions } from 'it-length-prefixed'

only help in checking the type but it don't include it in the actual code...

@achingbrain
Copy link
Member

achingbrain commented Feb 18, 2025

import type { DecoderOptions as LpDecoderOptions } from 'it-length-prefixed'

and

import { type DecoderOptions as LpDecoderOptions } from 'it-length-prefixed'

are the same thing, we've just settled on the former in the codebase (for the most part) though vscode defaults to the latter if no existing import is present 🤷

All types are removed from the code at compile time since js has no types so there's no way to express them.

Copy link
Contributor Author

@acul71 acul71 left a comment

Choose a reason for hiding this comment

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

About

if (this._rawOutboundStream != null)

yes this better

this._rawOutboundStream?.closeWrite()

About

    async function * (source) {
       for await (const chunk of source) {
          yield chunk
        }
      },

This is because when I started to write the test I thought the the receiving peer was not important because I wanted to test if it gave an error in encoding, but it was not working so I added the code with console.log (that I've removed before submitting) to test why it was not working then I realized that if I didn't add the receiving host it wasn't working but then I just removed the console.log and not the yield stuff

@achingbrain
Copy link
Member

this._rawOutboundStream?.closeWrite()

Yeah, normally it would be picked up by the linter but there was a comment disabling the linter on that line.

I just removed the console.log and not the yield stuff

Fair enough, I thought it might have been something like that 👍

@achingbrain achingbrain merged commit e7e2802 into libp2p:main Feb 18, 2025
34 checks passed
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