Skip to content

src: sync access for report and openssl options - #32618

Closed
sam-github wants to merge 1 commit into
nodejs:masterfrom
sam-github:more-locks
Closed

src: sync access for report and openssl options#32618
sam-github wants to merge 1 commit into
nodejs:masterfrom
sam-github:more-locks

Conversation

@sam-github

Copy link
Copy Markdown
Contributor

Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.

Also documented existence and typical use of the per-process cli option
mutex.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Apr 2, 2020
@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2020
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Comment threadsrc/node_options.h Outdated
@gireeshpunathil

Copy link
Copy Markdown
Member

(Outside of this PR, but because we are here) does it makes sense to have an internal document which contain:

  • shared data
  • different locks that are used
  • lock ordering constraints if any

@codecov-io

codecov-io commented Apr 3, 2020

Copy link
Copy Markdown

Codecov Report

Merging #32618 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #32618 +/- ##
=======================================
Coverage 97.54% 97.55% =======================================
Files 195 195 Lines 65834 65811 -23 =======================================
- Hits 64218 64201 -17 + Misses 1616 1610 -6 
Impacted FilesCoverage Δ
lib/_stream_writable.js99.03% <0.00%> (-0.01%)⬇️
lib/_stream_readable.js98.56% <0.00%> (-0.01%)⬇️
lib/internal/http2/core.js96.92% <0.00%> (+0.05%)⬆️
lib/internal/util/inspect.js100.00% <0.00%> (+0.09%)⬆️
lib/internal/stream_base_commons.js98.57% <0.00%> (+0.68%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78e6d24...80bf18b. Read the comment docs.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

addaleax pushed a commit that referenced this pull request Apr 8, 2020
Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
PR-URL: #32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax

Copy link
Copy Markdown
Member

Landed in fcde1de

@addaleaxaddaleax closed this Apr 8, 2020
targos pushed a commit that referenced this pull request Apr 12, 2020
Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
PR-URL: #32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@targostargos mentioned this pull request Apr 13, 2020
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
PR-URL: #32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@targostargos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
PR-URL: nodejs#32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Audited usage of per-process OpenSSL and Report options, adding two
required mutexes.
Also documented existence and typical use of the per-process cli option
mutex.
PR-URL: #32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@targostargos mentioned this pull request May 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@sam-github@nodejs-github-bot@gireeshpunathil@codecov-io@addaleax@jasnell@cjihrig@targos