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

noc/uninitialized_value_concatenation #97

Conversation

denissafiullin-deriv
Copy link
Contributor

@denissafiullin-deriv denissafiullin-deriv commented Nov 29, 2023

Currently $log->debug produces a warning due to the uninitialised value of $c->tx->remote_address. During the last month, there were 277 warn logs out of 1.04 million connections, which is 0.027 %. To solve the issue, the following steps were taken:

  1. Use Log::Any instead of Mojo::Log to be able to use debugf instead of debug and to follow the Perl Style Guide.
  2. Avoid string interpolation as discussed here
  3. Use single quotes instead of double quotes, as interpolation is not required
  4. Fallback to string undef if remote_address wasn't resolved.

ClickUp card: https://app.clickup.com/t/20696747/PLA-768

Copy link

@review-deriv review-deriv left a comment

Choose a reason for hiding this comment

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

PR description is too short. You can use this format:

# Description
This is a summary of the changes. This should describe why do we need this change.
Whys and Hows come here.

# Cards
* https://clickup.com/12345
* https://clickup.com/12346

Copy link

@review-deriv review-deriv left a comment

Choose a reason for hiding this comment

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

PR description is too short. You can use this format:

# Description
This is a summary of the changes. This should describe why do we need this change.
Whys and Hows come here.

# Cards
* https://clickup.com/12345
* https://clickup.com/12346

@@ -51,7 +51,7 @@ sub open_connection {
my ($c) = @_;

my $log = $c->app->log;
$log->debug("accepting a websocket connection from " . $c->tx->remote_address);
$log->debug("accepting a websocket connection from " . ($c->tx->remote_address // 'undefined remote address'));

Choose a reason for hiding this comment

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

can we use debugf here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, replaced with debugf

$log->debug("accepting a websocket connection from " . $c->tx->remote_address);
my $log = $c->app->log;
my $remote_address = $c->tx->remote_address // 'undef';
$log->debug('accepting a websocket connection from ' . $remote_address);
Copy link
Contributor

Choose a reason for hiding this comment

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

log::any format log can handle undef

Suggested change
$log->debug('accepting a websocket connection from ' . $remote_address);
$log->debugf('accepting a websocket connection from %s', $c->tx->remote_address);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Introduced Log::Any

@aisha-deriv
Copy link
Contributor

@denissafiullin-deriv please add changes file

    Mojo::WebSocketProxy::Dispatcher changes:
    - Use Log::Any instead of Mojo::Log in Dispatcher;
    - Avoid string interpolation to solve warning due to the uninitialised value of $c->tx->remote_address;
    - Add fallback value for cases when remote_address can't be resolved;
@release-write-deriv release-write-deriv merged commit 7a9f49b into binary-com:master Dec 19, 2023
2 checks passed
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.

None yet

7 participants