Skip to content
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

Proposal: Splitting up node_crypto.cc #16524

Closed
jasnell opened this issue Oct 26, 2017 · 6 comments
Closed

Proposal: Splitting up node_crypto.cc #16524

jasnell opened this issue Oct 26, 2017 · 6 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@jasnell
Copy link
Member

jasnell commented Oct 26, 2017

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
@jasnell jasnell added the crypto Issues and PRs related to the crypto subsystem. label Oct 26, 2017
@jasnell
Copy link
Member Author

jasnell commented Oct 26, 2017

Ping @nodejs/crypto

@bnoordhuis
Copy link
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.

@tniessen
Copy link
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.

@jasnell jasnell closed this as completed Jun 29, 2018
@tniessen
Copy link
Member

tniessen commented Jul 2, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Jul 2, 2018

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

@ryzokuken
Copy link
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
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants