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

src: refactor crypto code with RAII cleanup #23014

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

gireeshpunathil
Copy link
Member

use more idiomatic expressions with RAII primitives, instead of old style goto

motivated by @addaleax 's efforts on the same, such as in #22981

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-bot nodejs-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 Sep 22, 2018
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I'd suggest to rename fail to failed which would make the added comment redundant IMO.

@gireeshpunathil
Copy link
Member Author

@tniessen - thanks, done.

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 23, 2018
@gireeshpunathil
Copy link
Member Author

looks like an unrelated flake in the CI:

17:56:26 not ok 154 parallel/test-bootstrap-modules
17:56:26   ---
17:56:26   duration_ms: 0.330
17:56:26   severity: fail
17:56:26   exitcode: 1
17:56:26   stack: |-
17:56:26     
17:56:26     events.js:167
17:56:26           throw er; // Unhandled 'error' event
17:56:26           ^
17:56:26     AssertionError [ERR_ASSERTION]: Internal Binding contextify,Internal Binding worker,NativeModule events,Internal Binding trace_events,NativeModule internal/safe_globals,NativeModule internal/async_hooks,NativeModule internal/errors,Internal Binding uv,Binding buffer,Internal Binding async_wrap,Binding config,Binding icu,NativeModule util,NativeModule internal/validators,NativeModule internal/encoding,NativeModule internal/util,Binding constants,Internal Binding util,NativeModule internal/util/types,Internal Binding types,NativeModule buffer,NativeModule internal/buffer,NativeModule internal/process/per_thread,NativeModule internal/process/worker_thread_only,NativeModule internal/process/stdio,NativeModule internal/worker,NativeModule assert,NativeModule internal/assert,NativeModule fs,NativeModule path,NativeModule internal/constants,Binding fs,NativeModule internal/fs/utils,NativeModule internal/url,NativeModule internal/querystring,Internal Binding url,NativeModule stream,NativeModule internal/streams/pipeline,NativeModule internal/streams/end-of-stream,NativeModule internal/streams/legacy,NativeModule _stream_readable,NativeModule internal/streams/buffer_list,NativeModule internal/streams/destroy,NativeModule internal/streams/state,NativeModule _stream_writable,NativeModule _stream_duplex,NativeModule _stream_transform,NativeModule _stream_passthrough,Internal Binding messaging,Internal Binding symbols,NativeModule internal/error-serdes,NativeModule v8,Internal Binding serdes,Internal Binding v8,NativeModule url,NativeModule internal/process/warning,NativeModule internal/process/next_tick,NativeModule internal/process/promises,NativeModule internal/fixed_queue,Internal Binding performance,NativeModule internal/inspector_async_hook,Binding inspector,Internal Binding options,NativeModule timers,Internal Binding timers,NativeModule internal/linkedlist,NativeModule internal/priority_queue,NativeModule internal/timers,NativeModule internal/modules/cjs/loader,NativeModule vm,NativeModule internal/modules/cjs/helpers,NativeModule console,NativeModule internal/queue_microtask,NativeModule async_hooks,NativeModule internal/domexception,NativeModule worker_threads,NativeModule module
17:56:26         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-bootstrap-modules.js:14:1)
17:56:26         at Module._compile (internal/modules/cjs/loader.js:703:30)
17:56:26         at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
17:56:26         at Module.load (internal/modules/cjs/loader.js:613:32)
17:56:26         at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
17:56:26         at Function.Module._load (internal/modules/cjs/loader.js:544:3)
17:56:26         at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
17:56:26         at MessagePort.port.on (internal/worker.js:447:27)
17:56:26         at MessagePort.emit (events.js:182:13)
17:56:26         at MessagePort.onmessage (internal/worker.js:67:8)
17:56:26     Emitted 'error' event at:
17:56:26         at Worker.[kOnErrorMessage] (internal/worker.js:319:10)
17:56:26         at Worker.[kOnMessage] (internal/worker.js:329:37)
17:56:26         at MessagePort.Worker.(anonymous function).on (internal/worker.js:266:57)
17:56:26         at MessagePort.emit (events.js:182:13)
17:56:26         at MessagePort.onmessage (internal/worker.js:67:8)
17:56:26   ...

@addaleax - do you know if it is known (and / or captured elsewhere) and can be ignored here?

@devsnek
Copy link
Member

devsnek commented Sep 25, 2018

@gireeshpunathil that failure was caused by another change and was fixed. you should be able to rebuild the worker job without problems.

@danbev
Copy link
Contributor

danbev commented Sep 26, 2018

Re-run of failing node-test-commit-custom-suites-freestyle.

@gireeshpunathil
Copy link
Member Author

second time failure in parallel/test-bootstrap-modules . @devsnek @joyeecheung any guidance here?

@joyeecheung
Copy link
Member

joyeecheung commented Sep 27, 2018

@gireeshpunathil Can you try rebasing onto the current master and start a fresh CI?

@gireeshpunathil
Copy link
Member Author

use more idiomatic expressions with RAII primitives,
instead of old style goto

PR-URL: nodejs#23014

Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gireeshpunathil gireeshpunathil merged commit 406e3ad into nodejs:master Sep 28, 2018
@tniessen tniessen mentioned this pull request Sep 28, 2018
2 tasks
targos pushed a commit that referenced this pull request Sep 28, 2018
use more idiomatic expressions with RAII primitives,
instead of old style goto

PR-URL: #23014

Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Oct 3, 2018
use more idiomatic expressions with RAII primitives,
instead of old style goto

PR-URL: #23014

Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.