Пробрасывание информации об ошибке через ВызватьИсключение #1488
Conversation
WalkthroughThe pull request introduces changes to the exception handling mechanism in the Changes
Sequence DiagramsequenceDiagram
participant MachineInstance
participant ExceptionHandler
participant ParametrizedRuntimeException
alt Is Error Template
MachineInstance->>ExceptionHandler: Check IsErrorTemplate
ExceptionHandler-->>ParametrizedRuntimeException: Create with template info
else Not Error Template
MachineInstance->>ExceptionHandler: Process non-template exception
ExceptionHandler-->>ParametrizedRuntimeException: Create with direct exception info
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/ScriptEngine/Machine/MachineInstance.cs (1)
1253-1259: LGTM! The enhanced exception handling logic improves error information propagation.The changes properly distinguish between error templates and regular exceptions, ensuring that error information is correctly preserved and propagated. Consider extracting the exception creation logic into separate methods for better readability.
Consider this refactoring to improve readability:
- if (exceptionValue is ExceptionInfoContext { IsErrorTemplate: true } excInfoTemplate) - { - throw new ParametrizedRuntimeException(excInfoTemplate.Description, excInfoTemplate.Parameters); - } - else if (exceptionValue is ExceptionInfoContext { IsErrorTemplate: false } excInfo) - { - throw new RuntimeException(excInfo.MessageWithoutCodeFragment, excInfo.ActualException()); - } + if (exceptionValue is ExceptionInfoContext excInfo) + { + if (excInfo.IsErrorTemplate) + { + throw CreateParametrizedRuntimeException(excInfo); + } + throw CreateRuntimeExceptionWithCause(excInfo); + }Add these private methods:
private static ParametrizedRuntimeException CreateParametrizedRuntimeException(ExceptionInfoContext excInfo) { return new ParametrizedRuntimeException(excInfo.Description, excInfo.Parameters); } private static RuntimeException CreateRuntimeExceptionWithCause(ExceptionInfoContext excInfo) { return new RuntimeException(excInfo.MessageWithoutCodeFragment, excInfo.ActualException()); }tests/global-funcs.os (1)
892-926: LGTM! The test provides good coverage of the new exception handling behavior.The test effectively validates:
- Initial exception throwing
- Error information propagation
- Re-throwing with preserved information
Consider adding these additional test cases:
- Nested exception handling (try-catch inside try-catch)
- Exception with empty or null parameters
- Exception with non-string parameters
- Multiple re-throws to verify deep propagation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/ScriptEngine/Machine/Contexts/ExceptionInfoContext.cs(1 hunks)src/ScriptEngine/Machine/MachineInstance.cs(1 hunks)tests/global-funcs.os(2 hunks)
🔇 Additional comments (2)
src/ScriptEngine/Machine/Contexts/ExceptionInfoContext.cs (1)
Line range hint
54-65: LGTM! The change in visibility is necessary for enhanced error handling.Making
ActualExceptionpublic allows external access to the underlying exception, which is essential for the improved error information propagation mechanism.tests/global-funcs.os (1)
80-81: LGTM! Test registration is properly added.The new test is correctly registered in the test list.
sfaqer
changed the title
| юТест.ПроверитьИстину(ЗначениеЗаполнено(ИнформацияОбОшибке), "Исключение не было брошено"); | ||
|
|
||
| Попытка | ||
| ВызватьИсключение ИнформацияОбОшибке; |
There was a problem hiding this comment.
Я не уверен, что это правильный API для этой функции. Давайте обсудим. @nixel2007 ?
There was a problem hiding this comment.
Ну я это делал из соображений того что это не отрывает обратную совместимость, и в общем случае достаточно удобно в сценарии работы с фоновыми заданиями, типа:
Дождались = Задание.ОжидатьЗавершения(Таймаут);
Если Дождались И Задание.Состояние = СостояниеФоновогоЗадания.Завершено Тогда
Возврат Задание.Результат;
ИначеЕсли Не Дождались Тогда
ВызватьИсключение "Превышено время ожидания получения результата";
Иначе
ВызватьИсключение Задание.ИнформацияОбОшибке;
КонецЕсли;Таким образом с помощью данной доработки, мы на верх по стеку во первых передали информацию о текущей ошибке со стеком текущего треда, так и внутрь положили причину с оригинальной информацией об ошибке из треда фонового задания, и со стеком треда фонового задания.
Если рассуждать в контексте API, то наверное можно сделать третий параметр конструктора шаблона информации об ошибке, типа:
Попытка
ВызватьИсключение "Я вложенная ошибка";
Исключение
ВызватьИсключение Новый ИнформацияОбОшибке("Я ошибка", Новый Массив, ИнформацияОбОшибке());
КонецПопыткиНо даже если добавить параметр Причина в конструктор, я бы всё равно голосовал за то что бы сохранить и то что есть в этом реквесте, т.к мне кажется что он дополняет контекст в части существующих сценариев обработки ошибок и не ломает обратную совместимость =)
There was a problem hiding this comment.
я ожидал ВызватьИсключение Новый ИнформацияОбОшибке(), но действительно, если уже есть сохраненная информация, то дать возможность сделать ее re-throw тоже нужно
There was a problem hiding this comment.
Мне синтаксис с пробросом имеющейся переменной ИнформацияОбОшибке непонятен. Неясно, что выбрасывается. С Новый и параметром конструктора - все понятно
There was a problem hiding this comment.
try {
} catch (Exception e) {
log.error("ой ой", e);
throw e;
} Тоже самое, не?
There was a problem hiding this comment.
Ну я выше расписал кейс с ФоновымЗданием, которое нам тоже не швыряет исключение, а приносит его в поле, как по другому сделать rethrow ИнформацииОбОшибке которая лежит в поле фонового задания?
There was a problem hiding this comment.
как по другому сделать rethrow ИнформацииОбОшибке которая лежит в поле фонового задания?
через Новый и вкладывание вероятно.
Но вы, раз ява-положительные, вы и скажите - такое правда норм, поймать в одном месте, записать e в переменную, передать в какой-то метод, а оттуда выбросить, как rethrow?
в переменную оно кладется просто в силу синтаксиса языка. передача в другой метод, наверное, не частая штука, но re-throw не из блока catch я и видел и сам писал.
There was a problem hiding this comment.
Я вынес доработку по конструктору в отдельный PR: #1494 по нему я так понял концептуальных возражений нет
В рамках этого PR и текущего обсуждения надо решить будем ли мы давать возможность писать:
ВызватьИсключение ИнформацияОбОшибке;Вместо
ВызватьИсключение Новый ИнформацияОбОшибке(ИнформацияОбОшибке.Описание, Новый Массив, ИнформацияОбОшибке);Или это будет слишком диабетический сахар xD
There was a problem hiding this comment.
Ну вот кейс с фоновым заданием и перевыбросом мне понятен. Теперь я согласен :)
There was a problem hiding this comment.
Перебазировал на актуальный develop и актуализировал реализацию по доработкам по конструктору
…стве параметра, создаётся новая `ИнформацияОбОшибке` которая содержит оригинальную информацию об ошибке в поле `Причина`
sfaqer
force-pushed
the
feature/excInfoRethrow
branch
from
07dbe5b to
3f7c479
Compare
Теперь при выбросе исключения с передачей
ИнформацииОбОшибкев качестве параметра, создаётся новаяИнформацииОбОшибкекоторая с описанием оригинальной информации об ошибке и которая содержит оригинальную информацию об ошибке вПричинаSummary by CodeRabbit
New Features
Bug Fixes