Skip to content

Proposal: Splitting up node_crypto.cc #16524

Closed

Description

@jasnell

The src/node_crypto.cc file is currently 6k+ lines of code that provides process.binding('crypto'). This one internal binding provides an array of crypto functions include Signing, Verification, Hash, limited key-generation, SecureContext for TLS connections, etc. As such, this one file has a LOT of code in it and it is quite difficult from a maintenance perspective.

Looking it over, I'd like to refactor both the source files and the process.binding() for crypto such that each individual subcomponent of node_crypto.cc is moved to it's own src/node_crypto_nnnn.cc file with it's own distinct process.binding('crypto-nnnn'). This will make it easier for us to maintain and update individual pieces of the code.

This would be a fairly large and time consuming refactoring so I wanted to make sure that folks are on board with it before I started down that path.

FWIW, there are two efforts prompting this:

  1. The migration to internal/errors and the pain involved with digging through this file to do so
  2. The investigation of promises support for the crypto module

Activity

  1. added
    cryptoIssues and PRs related to the crypto subsystem.
    on Oct 26, 2017
  2. jasnell commented on Oct 26, 2017

    @jasnell
    MemberAuthor

    Ping @nodejs/crypto

  3. bnoordhuis commented on Oct 27, 2017

    @bnoordhuis
    Member

    I'm not bothered by the file size, that doesn't really impact maintainability and I don't really see how breaking it into smaller files would help.

    That said, I'm not opposed unless it means I need to review big, boring pull requests.

  4. tniessen commented on Oct 27, 2017

    @tniessen
    Member

    such that each individual subcomponent of node_crypto.cc is moved to it's own src/node_crypto_nnnn.cc file with it's own distinct process.binding('crypto-nnnn').

    I am okay with such a change as long as there are not too many (small) submodules, but I would generally prefer to break it down.

  5. tniessen commented on Jul 2, 2018

    @tniessen
    Member

    @jasnell Was this closed due to inactivity? @ryzokuken brought this up in #21288 (review) again.

  6. jasnell commented on Jul 2, 2018

    @jasnell
    MemberAuthor

    Yes, closed due to inactivity. It's fine to reopen if folks want to work on it

  7. ryzokuken commented on Jul 2, 2018

    @ryzokuken
    Contributor

    @jasnell I had been discussing with @tniessen about how the file had grown insanely big to the point that it's intimidating for people to "just dive into". Ideally, we should break the file into smaller chunks based on some criteria like you proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    cryptoIssues and PRs related to the crypto subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions