From 6abbdad0e59216b02fc14d45029b7b88c2d08c1a Mon Sep 17 00:00:00 2001 From: Andrew Hoskins Date: Tue, 28 Sep 2021 13:43:35 -0700 Subject: [PATCH 1/4] Gracefully degrade application failure error when deserializing --- lib/temporal/workflow/errors.rb | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/temporal/workflow/errors.rb b/lib/temporal/workflow/errors.rb index 8e917bee..13d0fafa 100644 --- a/lib/temporal/workflow/errors.rb +++ b/lib/temporal/workflow/errors.rb @@ -11,11 +11,32 @@ def self.generate_error(failure, default_exception_class = StandardError) case failure.failure_info when :application_failure_info exception_class = safe_constantize(failure.application_failure_info.type) - exception_class ||= default_exception_class + fallback_to_default = false + if exception_class.nil? + Temporal.logger.error( + "Could not find original error class. Defaulting to StandardError.", + {original_error: failure.application_failure_info.type}, + ) + exception_class = default_exception_class + end + message = from_details_payloads(failure.application_failure_info.details) - backtrace = failure.stack_trace.split("\n") - exception_class.new(message).tap do |exception| + begin + exception = exception_class.new(message) + rescue ArgumentError => deserialization_error + # We don't currently support serializing/deserializing exceptions with more than one argument. + exception = default_exception_class.new(message) + Temporal.logger.error( + "Could not instantiate original error. Defaulting to StandardError.", + { + original_error: failure.application_failure_info.type, + instantiation_error_message: deserialization_error.message, + }, + ) + end + exception.tap do |exception| + backtrace = failure.stack_trace.split("\n") exception.set_backtrace(backtrace) if !backtrace.empty? end when :timeout_failure_info From 56558cac1b47c681258dfa3799889f959db37851 Mon Sep 17 00:00:00 2001 From: Andrew Hoskins Date: Tue, 28 Sep 2021 13:50:53 -0700 Subject: [PATCH 2/4] Add original class to the message --- lib/temporal/workflow/errors.rb | 6 +- .../application_failure_fabricator.rb | 12 +++ .../unit/lib/temporal/workflow/errors_spec.rb | 86 +++++++++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 spec/fabricators/application_failure_fabricator.rb create mode 100644 spec/unit/lib/temporal/workflow/errors_spec.rb diff --git a/lib/temporal/workflow/errors.rb b/lib/temporal/workflow/errors.rb index 13d0fafa..d7c294f5 100644 --- a/lib/temporal/workflow/errors.rb +++ b/lib/temporal/workflow/errors.rb @@ -10,22 +10,24 @@ class Errors def self.generate_error(failure, default_exception_class = StandardError) case failure.failure_info when :application_failure_info + message = from_details_payloads(failure.application_failure_info.details) + exception_class = safe_constantize(failure.application_failure_info.type) - fallback_to_default = false if exception_class.nil? Temporal.logger.error( "Could not find original error class. Defaulting to StandardError.", {original_error: failure.application_failure_info.type}, ) + message = "#{failure.application_failure_info.type}: #{message}" exception_class = default_exception_class end - message = from_details_payloads(failure.application_failure_info.details) begin exception = exception_class.new(message) rescue ArgumentError => deserialization_error # We don't currently support serializing/deserializing exceptions with more than one argument. + message = "#{exception_class}: #{message}" exception = default_exception_class.new(message) Temporal.logger.error( "Could not instantiate original error. Defaulting to StandardError.", diff --git a/spec/fabricators/application_failure_fabricator.rb b/spec/fabricators/application_failure_fabricator.rb new file mode 100644 index 00000000..a4d74f30 --- /dev/null +++ b/spec/fabricators/application_failure_fabricator.rb @@ -0,0 +1,12 @@ +# Simulates Temporal::Connection::Serializer::Failure +Fabricator(:application_failure, from: Temporal::Api::Failure::V1::Failure) do + transient :error_class, :backtrace + message { |attrs| attrs[:message] } + stack_trace { |attrs| attrs[:backtrace].join("\n") } + application_failure_info do |attrs| + Temporal::Api::Failure::V1::ApplicationFailureInfo.new( + type: attrs[:error_class], + details: TestDeserializer.new.to_details_payloads(attrs[:message]), + ) + end +end diff --git a/spec/unit/lib/temporal/workflow/errors_spec.rb b/spec/unit/lib/temporal/workflow/errors_spec.rb new file mode 100644 index 00000000..aa306291 --- /dev/null +++ b/spec/unit/lib/temporal/workflow/errors_spec.rb @@ -0,0 +1,86 @@ +require 'temporal/workflow/errors' + +class TestDeserializer + include Temporal::Concerns::Payloads +end + +class ErrorWithTwoArgs < StandardError + def initialize(message, another_argument); end +end + +class SomeError < StandardError; end + +describe Temporal::Workflow::Errors do + describe '.generate_error' do + it "instantiates properly when the client has the error" do + message = "An error message" + stack_trace = ["a fake backtrace"] + failure = Fabricate( + :application_failure, + message: message, + backtrace: stack_trace, + error_class: SomeError.to_s + ) + + e = Temporal::Workflow::Errors.generate_error(failure) + expect(e).to be_a(SomeError) + expect(e.message).to eq(message) + expect(e.backtrace).to eq(stack_trace) + + end + + it "falls back to StandardError when the client doesn't have the error class" do + allow(Temporal.logger).to receive(:error) + + message = "An error message" + stack_trace = ["a fake backtrace"] + failure = Fabricate( + :application_failure, + message: message, + backtrace: stack_trace, + error_class: 'NonexistentError', + ) + + e = Temporal::Workflow::Errors.generate_error(failure) + expect(e).to be_a(StandardError) + expect(e.message).to eq("NonexistentError: An error message") + expect(e.backtrace).to eq(stack_trace) + expect(Temporal.logger) + .to have_received(:error) + .with( + 'Could not find original error class. Defaulting to StandardError.', + {:original_error=>"NonexistentError"}, + ) + + end + + + it "falls back to StandardError when the client doesn't have the error class" do + allow(Temporal.logger).to receive(:error) + + message = "An error message" + stack_trace = ["a fake backtrace"] + failure = Fabricate( + :application_failure, + message: message, + backtrace: stack_trace, + error_class: ErrorWithTwoArgs.to_s, + ) + + e = Temporal::Workflow::Errors.generate_error(failure) + expect(e).to be_a(StandardError) + expect(e.message).to eq("ErrorWithTwoArgs: An error message") + expect(e.backtrace).to eq(stack_trace) + expect(Temporal.logger) + .to have_received(:error) + .with( + 'Could not instantiate original error. Defaulting to StandardError.', + { + :original_error=>"ErrorWithTwoArgs", + :instantiation_error_message=> "wrong number of arguments (given 1, expected 2)", + }, + ) + end + + end +end From 37545a4ee3413d1edceac7425efc80da31d0c820 Mon Sep 17 00:00:00 2001 From: Andrew Hoskins Date: Tue, 28 Sep 2021 17:06:27 -0700 Subject: [PATCH 3/4] Fix copypasta --- spec/unit/lib/temporal/workflow/errors_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/lib/temporal/workflow/errors_spec.rb b/spec/unit/lib/temporal/workflow/errors_spec.rb index aa306291..9811ba39 100644 --- a/spec/unit/lib/temporal/workflow/errors_spec.rb +++ b/spec/unit/lib/temporal/workflow/errors_spec.rb @@ -55,7 +55,7 @@ class SomeError < StandardError; end end - it "falls back to StandardError when the client doesn't have the error class" do + it "falls back to StandardError when the client can't initialize the error class" do allow(Temporal.logger).to receive(:error) message = "An error message" From c0c5aa237a2d60649b3e6b8f9f18e1a435f40956 Mon Sep 17 00:00:00 2001 From: Andrew Hoskins Date: Thu, 30 Sep 2021 07:49:14 -0700 Subject: [PATCH 4/4] Feedback --- .../{ => grpc}/application_failure_fabricator.rb | 6 +++++- spec/unit/lib/temporal/workflow/errors_spec.rb | 16 ++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) rename spec/fabricators/{ => grpc}/application_failure_fabricator.rb (68%) diff --git a/spec/fabricators/application_failure_fabricator.rb b/spec/fabricators/grpc/application_failure_fabricator.rb similarity index 68% rename from spec/fabricators/application_failure_fabricator.rb rename to spec/fabricators/grpc/application_failure_fabricator.rb index a4d74f30..edf90c82 100644 --- a/spec/fabricators/application_failure_fabricator.rb +++ b/spec/fabricators/grpc/application_failure_fabricator.rb @@ -1,5 +1,9 @@ +require 'temporal/concerns/payloads' +class TestDeserializer + include Temporal::Concerns::Payloads +end # Simulates Temporal::Connection::Serializer::Failure -Fabricator(:application_failure, from: Temporal::Api::Failure::V1::Failure) do +Fabricator(:api_application_failure, from: Temporal::Api::Failure::V1::Failure) do transient :error_class, :backtrace message { |attrs| attrs[:message] } stack_trace { |attrs| attrs[:backtrace].join("\n") } diff --git a/spec/unit/lib/temporal/workflow/errors_spec.rb b/spec/unit/lib/temporal/workflow/errors_spec.rb index 9811ba39..a9e053f6 100644 --- a/spec/unit/lib/temporal/workflow/errors_spec.rb +++ b/spec/unit/lib/temporal/workflow/errors_spec.rb @@ -1,9 +1,5 @@ require 'temporal/workflow/errors' -class TestDeserializer - include Temporal::Concerns::Payloads -end - class ErrorWithTwoArgs < StandardError def initialize(message, another_argument); end end @@ -16,7 +12,7 @@ class SomeError < StandardError; end message = "An error message" stack_trace = ["a fake backtrace"] failure = Fabricate( - :application_failure, + :api_application_failure, message: message, backtrace: stack_trace, error_class: SomeError.to_s @@ -35,7 +31,7 @@ class SomeError < StandardError; end message = "An error message" stack_trace = ["a fake backtrace"] failure = Fabricate( - :application_failure, + :api_application_failure, message: message, backtrace: stack_trace, error_class: 'NonexistentError', @@ -49,7 +45,7 @@ class SomeError < StandardError; end .to have_received(:error) .with( 'Could not find original error class. Defaulting to StandardError.', - {:original_error=>"NonexistentError"}, + {original_error: "NonexistentError"}, ) end @@ -61,7 +57,7 @@ class SomeError < StandardError; end message = "An error message" stack_trace = ["a fake backtrace"] failure = Fabricate( - :application_failure, + :api_application_failure, message: message, backtrace: stack_trace, error_class: ErrorWithTwoArgs.to_s, @@ -76,8 +72,8 @@ class SomeError < StandardError; end .with( 'Could not instantiate original error. Defaulting to StandardError.', { - :original_error=>"ErrorWithTwoArgs", - :instantiation_error_message=> "wrong number of arguments (given 1, expected 2)", + original_error: "ErrorWithTwoArgs", + instantiation_error_message: "wrong number of arguments (given 1, expected 2)", }, ) end