-
Notifications
You must be signed in to change notification settings - Fork 592
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.
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
Consolidate/refactor local exceptions in C# #2338
Conversation
{ | ||
} | ||
|
||
public override string ice_id() => "::Ice::ObjectNotExistException"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use ice_id
for metrics and more.
- Consolidate and refactor the exceptions derived from LocalException. | ||
| Local exception in Ice 3.7 | Replacement | | ||
|-------------------------------------|----------------------------| | ||
| BadMagicException | ProtocolException (base) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also useful as a guide when updating other language mappings.
setState(StateClosed, | ||
new ConnectionAbortedException( | ||
"Connection close forcefully by the application.", | ||
closedByApplication: true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is closeByApplication
ever false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently - this exception is only thrown when the application aborts the connection "manually".
We need t closedByApplication
to not retry a connection that was closed manually - that's the existing retry logic.
What about replacing ConnectionIdleException by ConnectionAbortedException with message + closedByApplication = false?
ConnectionLostException (a SocketException) also means "connection aborted". Should we replace it by ConnectionAbortedException?
All for a follow-up PR.
This PR reworks the local exceptions in C#. It should be a model for the other language mappings.
In particular: