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.

Sign up for GitHub

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

Пробрасывание информации об ошибке через ВызватьИсключение #1488

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Пробрасывание информации об ошибке через ВызватьИсключение #1488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/ScriptEngine/Machine/MachineInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,12 +1250,20 @@ private void RaiseException(int arg)
else
{
var exceptionValue = _operationStack.Pop().GetRawValue();
if (exceptionValue is ExceptionInfoContext { IsErrorTemplate: true } excInfo)
if (exceptionValue is ExceptionInfoContext { IsErrorTemplate: true } excTemplateInfo)
{
throw new ParametrizedRuntimeException(
excTemplateInfo.Description,
excTemplateInfo.Parameters,
excTemplateInfo.InnerException
);
}
else if (exceptionValue is ExceptionInfoContext { IsErrorTemplate: false } excInfo)
{
throw new ParametrizedRuntimeException(
excInfo.Description,
excInfo.Parameters,
excInfo.InnerException
ValueFactory.Create(),
excInfo
);
}
else
Expand Down
36 changes: 36 additions & 0 deletions tests/global-funcs.os
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
ВсеТесты.Добавить("Тест_ДолженПроверитьКраткоеПредставлениеОшибки");
ВсеТесты.Добавить("Тест_ДолженПроверитьПодробноеПредставлениеОшибки");
ВсеТесты.Добавить("Тест_ДолженПроверитьИнформацияОбОшибкеСПричиной");
ВсеТесты.Добавить("Тест_ДолженПроверитьИнформацияОбОшибкеПробрасываетсяПриВызовеИсключения");

ВсеТесты.Добавить("Тест_ДолженПроверитьОбъединениеПутей");

Expand Down Expand Up @@ -933,6 +934,41 @@

КонецПроцедуры

Процедура Тест_ДолженПроверитьИнформацияОбОшибкеПробрасываетсяПриВызовеИсключения() Экспорт

Попытка
ВыброситьТестовоеИсключение();
Исключение
ИнформацияОбОшибке = ИнформацияОбОшибке();
КонецПопытки;

юТест.ПроверитьИстину(ЗначениеЗаполнено(ИнформацияОбОшибке), "Исключение не было брошено");

Попытка
ВызватьИсключение ИнформацияОбОшибке;
Copy link
Owner

Choose a reason for hiding this comment

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

Я не уверен, что это правильный API для этой функции. Давайте обсудим. @nixel2007 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ну я это делал из соображений того что это не отрывает обратную совместимость, и в общем случае достаточно удобно в сценарии работы с фоновыми заданиями, типа:

	Дождались = Задание.ОжидатьЗавершения(Таймаут);

	Если Дождались И Задание.Состояние = СостояниеФоновогоЗадания.Завершено Тогда
		Возврат Задание.Результат;
	ИначеЕсли Не Дождались Тогда
		ВызватьИсключение "Превышено время ожидания получения результата";
	Иначе
		ВызватьИсключение Задание.ИнформацияОбОшибке;
	КонецЕсли;

Таким образом с помощью данной доработки, мы на верх по стеку во первых передали информацию о текущей ошибке со стеком текущего треда, так и внутрь положили причину с оригинальной информацией об ошибке из треда фонового задания, и со стеком треда фонового задания.

Если рассуждать в контексте API, то наверное можно сделать третий параметр конструктора шаблона информации об ошибке, типа:

Попытка
    ВызватьИсключение "Я вложенная ошибка";
Исключение
    ВызватьИсключение Новый ИнформацияОбОшибке("Я ошибка", Новый Массив, ИнформацияОбОшибке());
КонецПопытки

Но даже если добавить параметр Причина в конструктор, я бы всё равно голосовал за то что бы сохранить и то что есть в этом реквесте, т.к мне кажется что он дополняет контекст в части существующих сценариев обработки ошибок и не ломает обратную совместимость =)

Copy link
Collaborator

Choose a reason for hiding this comment

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

я ожидал ВызватьИсключение Новый ИнформацияОбОшибке(), но действительно, если уже есть сохраненная информация, то дать возможность сделать ее re-throw тоже нужно

Copy link
Owner

Choose a reason for hiding this comment

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

Мне синтаксис с пробросом имеющейся переменной ИнформацияОбОшибке непонятен. Неясно, что выбрасывается. С Новый и параметром конструктора - все понятно

Copy link
Collaborator

Choose a reason for hiding this comment

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

try {

} catch (Exception e) {
  log.error("ой ой", e);
  throw e;
}

Тоже самое, не?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ну я выше расписал кейс с ФоновымЗданием, которое нам тоже не швыряет исключение, а приносит его в поле, как по другому сделать rethrow ИнформацииОбОшибке которая лежит в поле фонового задания?

Copy link
Collaborator

Choose a reason for hiding this comment

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

как по другому сделать rethrow ИнформацииОбОшибке которая лежит в поле фонового задания?

через Новый и вкладывание вероятно.

Но вы, раз ява-положительные, вы и скажите - такое правда норм, поймать в одном месте, записать e в переменную, передать в какой-то метод, а оттуда выбросить, как rethrow?

в переменную оно кладется просто в силу синтаксиса языка. передача в другой метод, наверное, не частая штука, но re-throw не из блока catch я и видел и сам писал.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я вынес доработку по конструктору в отдельный PR: #1494 по нему я так понял концептуальных возражений нет

В рамках этого PR и текущего обсуждения надо решить будем ли мы давать возможность писать:

ВызватьИсключение ИнформацияОбОшибке;

Вместо

ВызватьИсключение Новый ИнформацияОбОшибке(ИнформацияОбОшибке.Описание, Новый Массив, ИнформацияОбОшибке);

Или это будет слишком диабетический сахар xD

Copy link
Owner

Choose a reason for hiding this comment

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

Ну вот кейс с фоновым заданием и перевыбросом мне понятен. Теперь я согласен :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Перебазировал на актуальный develop и актуализировал реализацию по доработкам по конструктору

Исключение
ИнформацияОбОшибке = ИнформацияОбОшибке();

юТест.ПроверитьИстину(
СтрНайти(ИнформацияОбОшибке.Описание, "тест-тест-тест") > 0,
"Сообщение в проброшенном исключении должно содержать текст оригинального исключения"
);

юТест.ПроверитьТип(
ИнформацияОбОшибке.Причина,
"ИнформацияОбОшибке",
"У проброшенного исключения должна быть заполнена причина, оригинальным исключением"
);

юТест.ПроверитьИстину(
СтрНайти(ИнформацияОбОшибке.Причина.Описание, "тест-тест-тест") > 0,
"Оригинальное сообщение должно содержать текст исключения"
);

КонецПопытки;

КонецПроцедуры

Процедура Тест_ДолженПроверитьОбъединениеПутей() Экспорт
СИ = Новый СистемнаяИнформация();
Если Найти(СИ.ВерсияОС,"Windows") > 0 Тогда
Expand Down