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

Logger fallback to console.error if process._rawDebug is missing #592

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Dec 16, 2024

Duplicate of #590.
When running under Deno instead of NodeJS I get the following stack trace when the native extension is loaded:

N-API call failed: napi_create_threadsafe_function( env, node_rawdebug, NULL, resource_name, 0, 1, ctx, s_threadsafe_log_finalize, ctx, s_threadsafe_log_call, &ctx->log_drain)                                                                                                                                                                           
    @ /codebuild/output/src1849732470/src/aws-crt-nodejs/source/logger.c:277: napi_function_expected                                                   
Fatal error condition occurred in /codebuild/output/src1849732470/src/aws-crt-nodejs/source/logger.c:277: napi_create_threadsafe_function( env, node_rawdebug, NULL, resource
_name, 0, 1, ctx, s_threadsafe_log_finalize, ctx, s_threadsafe_log_call, &ctx->log_drain)                                                                                
Exiting Application

Deno does not provide console._rawDebug so fall back to console.error instead.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bretambrose
Copy link
Contributor

I don't think we should merge PRs containing code that never gets meaningfully exercised anywhere in testing or CI. I'm also not thrilled about adding support for non-Node execution engines.

@waahm7
Copy link
Contributor Author

waahm7 commented Dec 17, 2024

@bretambrose

I don't think we should merge PRs containing code that never gets meaningfully exercised anywhere in testing or CI. I'm also not thrilled about adding support for non-Node execution engines.

In general, I agree, but for this particular change, _rawDebug is a private function that we should not be using from the Node code [link].

// Most of the time, it's best to use `console.error` to write  
// to the `process.stderr` stream. However, in some cases, such as  
// when debugging the `stream.Writable` class or the `process.nextTick`  
// function, it is useful to bypass JavaScript entirely.

I don't know the Node/JavaScript world well, but I think console.error is recommended over process._rawDebug. Since process._rawDebug is an internal function, Node is free to remove or change its implementation in the future, so it seems reasonable to fall back to console.error if it is not found. I haven't made console.error the default due to performance concerns and not knowing enough to make this change.

I have tested the code in CI to ensure that everything works even if we always use console.error. However, I haven't tested it locally using Deno.

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.

3 participants