Skip to content

Commit

Permalink
Use a dedicated private sub inside try to avoid early return
Browse files Browse the repository at this point in the history 
Sort of early refactoring before replace Try::Tiny to Feature::Compat::Try

https://progress.opensuse.org/issues/176862
  • Loading branch information
@b10n1k
b10n1k committed Feb 28, 2025
1 parent 97f1965 commit f035983
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 68 deletions.
14 changes: 7 additions & 7 deletions lib/OpenQA/WebAPI/Controller/API/V1/JobTemplate.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

package OpenQA::WebAPI::Controller::API::V1::JobTemplate;
use Mojo::Base 'Mojolicious::Controller', -signatures;
use Try::Tiny;
use Feature::Compat::Try;
use OpenQA::App;
use OpenQA::YAML qw(load_yaml dump_yaml);
use List::Util qw(min);
Expand Down Expand Up @@ -251,23 +251,23 @@ sub update ($self) {
$user_errors
= $self->app->validate_yaml($data, $validation->param('schema'), $self->app->log->level eq 'debug');
}
catch {
catch ($e) {
# Push the exception to the list of errors without the trailing new line
push @$user_errors, substr($_, 0, -1);
};
push @$user_errors, substr($e, 0, -1);
}
return $self->respond_to(json => {json => {error => $user_errors}, status => 400}) if @$user_errors;

my $json = {};
my @server_errors;
try {
$self->_perform_update($id, $name, $json, $data, $yaml, $template_reference, $to_expand, $user_errors);
}
catch {
catch ($e) {
# Push the exception to the list of errors without the trailing new line
my $error = substr($_, 0, -1);
my $error = substr($e, 0, -1);
my $error_type = ($error =~ qr/unique constraint/) ? $user_errors : \@server_errors;
push @$error_type, $error unless $error eq 'abort transaction';
};
}

if (@server_errors) {
push @$user_errors, 'Internal server error occurred';
Expand Down
125 changes: 64 additions & 61 deletions lib/OpenQA/WebSockets/Controller/Worker.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# SPDX-License-Identifier: GPL-2.0-or-later

package OpenQA::WebSockets::Controller::Worker;
use Mojo::Base 'Mojolicious::Controller';
use Mojo::Base 'Mojolicious::Controller', -signatures;

use DBIx::Class::Timestamps 'now';
use OpenQA::Schema;
Expand All @@ -13,13 +13,12 @@ use OpenQA::Jobs::Constants;
use OpenQA::Scheduler::Client;
use DateTime;
use Data::Dump 'pp';
use Try::Tiny;
use Feature::Compat::Try;
use Mojo::Util 'dumper';

use constant LOG_WORKER_STATUS_MESSAGES => $ENV{OPENQA_LOG_WORKER_STATUS_MESSAGES} // 0;

sub ws {
my ($self) = @_;
sub ws ($self) {
my $status = $self->status;

# add worker connection
Expand All @@ -34,8 +33,7 @@ sub ws {
$self->tx->max_websocket_size(10485760);
}

sub _finish {
my ($self, $code, $reason) = @_;
sub _finish ($self, $code, $reason) {
return undef unless $self;

my $worker = OpenQA::WebSockets::Model::Status->singleton->remove_worker_connection($self->tx);
Expand All @@ -51,9 +49,7 @@ sub _finish {
# is lost unexpectedly. It will be considered offline after the configured timeout expires.
}

sub _message {
my ($self, $json) = @_;

sub _message ($self, $json) {
my $app = $self->app;
my $schema = $app->schema;
my $tx = $self->tx;
Expand Down Expand Up @@ -109,10 +105,10 @@ sub _message {
$_->reschedule_state for @jobs;
});
}
catch {
catch ($e) {
# uncoverable statement
log_warning("Unable to re-schedule job(s) $job_ids_str rejected by worker $worker_id: $_");
};
log_warning("Unable to re-schedule job(s) $job_ids_str rejected by worker $worker_id: $e");
}

# log that we 'saw' the worker
$worker_db->seen;
Expand Down Expand Up @@ -172,63 +168,27 @@ sub _message {
# Tell the worker that we saw it (used for tests and debugging)
$tx->send({json => {type => 'info', seen => 1}});
}
catch {
log_error("Failed updating seen and error status of worker $worker_id: $_"); # uncoverable statement
};
catch ($e) {
log_error("Failed updating seen and error status of worker $worker_id: $e"); # uncoverable statement
}
}

# find the job currently associated with that worker and check whether the worker still
# executes the job it is supposed to
my $current_job_id;
my ($current_job, $current_job_id, $unfinished_jobs);
try {
return undef unless $worker;

my $registered_job_token;
my $current_job_state;
my @unfinished_jobs = $worker->unfinished_jobs;
my $current_job = $worker->job // $unfinished_jobs[0];
if ($current_job) {
$current_job_id = $current_job->id;
$current_job_state = $current_job->state;
($current_job, $job_id, $unfinished_jobs)
= $self->_should_assign_jobs($worker, $worker_id, \$current_job_id, $job_id, $job_token,
$worker_previously_idle);
if (defined $unfinished_jobs) {
log_debug("Rescheduling jobs assigned to worker $worker_id");
$worker->reschedule_assigned_jobs([$current_job, @$unfinished_jobs]);
}

# log debugging info
log_trace("Found job $current_job_id in DB from worker_status update sent by worker $worker_id")
if defined $current_job_id;
log_trace("Received request has job id: $job_id")
if defined $job_id;
$registered_job_token = $worker->get_property('JOBTOKEN');
log_trace("Worker $worker_id for job $current_job_id has token $registered_job_token")
if defined $current_job_id && defined $registered_job_token;
log_trace("Received request has token: $job_token")
if defined $job_token;

# skip any further actions if worker just does the one job we expected it to do
return undef
if ( defined $job_id
&& defined $current_job_id
&& defined $job_token
&& defined $registered_job_token
&& $job_id eq $current_job_id
&& (my $job_token_correct = $job_token eq $registered_job_token)
&& OpenQA::Jobs::Constants::meta_state($current_job_state) eq OpenQA::Jobs::Constants::EXECUTION)
&& (scalar @unfinished_jobs <= 1);

# give worker a second chance to process the job assignment
# possible situation on the worker: The worker might be sending a status update claiming it is
# idle (or has doing that task piled up on the event loop). At the same time a job arrives. The
# message regarding that job will be processed after sending the idle status. So let's give the
# worker another change to process the message about its assigned job.
return undef unless $worker_previously_idle;

log_debug("Rescheduling jobs assigned to worker $worker_id");
$worker->reschedule_assigned_jobs([$current_job, @unfinished_jobs]);
}
catch {
catch ($e) {
# uncoverable statement
log_warning("Unable to verify whether worker $worker_id runs its job(s) as expected: $_");
};

log_warning("Unable to verify whether worker $worker_id runs its job(s) as expected: $e");
}
# consider the worker idle unless it claims to be broken or work on a job
$worker_status->{idle_despite_job_assignment}
= !$worker_is_broken && !defined $job_id && defined $current_job_id;
Expand All @@ -238,4 +198,47 @@ sub _message {
}
}

sub _should_assign_jobs ($self, $worker, $worker_id, $current_job_id, $job_id, $job_token, $worker_previously_idle) {
return undef unless $worker;

my $registered_job_token;
my $current_job_state;
my @unfinished_jobs = $worker->unfinished_jobs;
my $current_job = $worker->job // $unfinished_jobs[0];
if ($current_job) {
$$current_job_id = $current_job->id;
$current_job_state = $current_job->state;
}

# log debugging info
log_trace("Found job $current_job_id in DB from worker_status update sent by worker $worker_id")
if defined $current_job_id;
log_trace("Received request has job id: $job_id")
if defined $job_id;
$registered_job_token = $worker->get_property('JOBTOKEN');
log_trace("Worker $worker_id for job $current_job_id has token $registered_job_token")
if defined $current_job_id && defined $registered_job_token;
log_trace("Received request has token: $job_token")
if defined $job_token;

# skip any further actions if worker just does the one job we expected it to do
return undef
if ( defined $job_id
&& defined $current_job_id
&& defined $job_token
&& defined $registered_job_token
&& $job_id eq $current_job_id
&& (my $job_token_correct = $job_token eq $registered_job_token)
&& OpenQA::Jobs::Constants::meta_state($current_job_state) eq OpenQA::Jobs::Constants::EXECUTION)
&& (scalar @unfinished_jobs <= 1);

# give worker a second chance to process the job assignment
# possible situation on the worker: The worker might be sending a status update claiming it is
# idle (or has doing that task piled up on the event loop). At the same time a job arrives. The
# message regarding that job will be processed after sending the idle status. So let's give the
# worker another change to process the message about its assigned job.
return undef unless $worker_previously_idle;
return ($current_job, $job_id, \@unfinished_jobs);
}

1;

0 comments on commit f035983

Please sign in to comment.