💾 Archived View for thrig.me › blog › 2022 › 12 › 15 › code-review.gmi captured on 2024-05-12 at 15:39:30. Gemini links have been rewritten to link to archived content

-=-=-=-=-=-=-

Code Review

The Code

It was claimed that chatgpt produced this code. Regardless of whether that is true, the code is not good.

     1  #!/usr/bin/perl
     2  
     3  use strict;
     4  use warnings;
     5  use POSIX ":sys_wait_h";
     6  
     7  my $pid = fork();
     8  
     9  if ($pid == 0) {
    10      # this is the child process
    11      sleep(30);
    12      kill("USR1", getppid());
    13      exit(0);
    14  } else {
    15      # this is the parent process
    16      local $SIG{USR1} = sub {
    17          print "Received SIGUSR1 signal from child.\n";
    18      };
    19      while (1) {
    20          print scalar(localtime()), "\n";
    21          sleep(1);
    22      }
    23  }

Errors

line 2 - no documentation summarizing what the code is supposed to do.
line 5 - wastes CPU and I/O pulling in an unused module.
line 7 - fork() can fail; this failure is not checked for.

I would not let this code past a code review.

Concerns

line 1 - /usr/bin/perl may be bad; perl may not be installed there. #!perl would be suitable for a script shipped with a module, in which case when the module is installed the correct path to the perl is set. Otherwise, #!/usr/bin/env perl is typical for portability on modern unix systems.
line 11 - sleep(30) is a race condition; it's a kluge to try to ensure that the child signal only happens after the parent gets the signal handler into place. A pipe is a better way for one process to notify another that it is ready.
line 17 - the message is misleading; the USR1 may not have come from the child process.
line 19 - the code does not exit after the signal; is it supposed to run forever? This is where the missing documentation on line 2 might help explain what the interface is supposed to be.
line 20 - the I/O assumes line buffered output; the timestamp will not be printed every second if stdout is block buffered. There should be a STDOUT->flush after each print, or STDOUT->autoflush(1) should be set if the I/O must always be interactive.

Rewrite

     1  #!/usr/bin/env perl
     2  # forkpractice - wait for a USR1 in parent sent by child and exit
     3  
     4  use strict;
     5  use warnings;
     6  use feature 'say';
     7  use IO::Pipe;
     8  use POSIX '_exit';
     9  use Time::Piece;
    10  
    11  my $pipe = IO::Pipe->new;
    12  my $pid  = fork() // die "fork failed: $!\n";
    13  
    14  if ( $pid == 0 ) {    # child
    15      $pipe->reader;
    16      sysread $pipe, my $ch, 1;    # block until parent ready
    17      kill USR1 => getppid;
    18      _exit(0);
    19  
    20  } else {    # parent
    21      STDOUT->autoflush(1);
    22      $pipe->writer;
    23      my $looping = 1;
    24      $SIG{USR1} = sub { say "received SIGUSR1"; $looping = 0 };
    25  
    26      my $odds = 0;
    27      while ($looping) {
    28          say localtime->strftime("%H:%M:%S");
    29          if ( rand() < $odds ) {
    30              say "closing pipe...";
    31              close $pipe;
    32          }
    33          sleep 1;
    34          $odds += 0.01;
    35      }
    36  }

Commentary

I prefer to keep most of the comments or documentation separate from the code, e.g. any Plain Old Documentation (POD), not present here, will be placed at the end of the file. Others mix POD into the script itself; I find this distracts from the code and makes searches more difficult: with the code and documentation distinct, you can stop a code search when the search wanders into the end-of-file documentation.

line 1 - env is used for portability on modern systems; the user need only alter the PATH environment variable to pick up on a different perl version.
line 2 - a quick summary of what the code is supposed to do. This helps orient the reader, usually future me. Not present is any sort of copyright or license spam, though others do like to include that.
line 11 - IO::Pipe hides a bunch of boilerplate but creates a different interface for pipe than one would employ in C.
line 18 - the use of exit or _exit is probably not important for this script, though can be in other code. Defensive coding might anticipate copy and paste from a good context into a bad one and guard against it.
line 24 - signal handlers that do as little as possible are good, as there can arise various complications within signal handlers. That is, try to only set flags that are acted on elsewhere, and do avoid complicated things like fiddling around in a database or exec'ing a new process.
lines 26, 29, 34 - the odds of triggering the signal vary over time; this is common in games and goes by the name of "rubber band odds" (Brogue) or "pseudo random distribution". Here the odds are set low--they could even be set to a negative value--to lower the odds of the trigger firing right away, but to increase the odds that the trigger will eventually fire. It is rare, but RNG can go on long streaks that may be unsuitable.
line 27 - the loop and thus the program goes away after the signal, sparing the user the need to kill the script.
line 28 - Time::Piece is used to customize the timestamp, e.g. to use ISO 8601, which sorts well. However, some Americans may want their weird American date format.