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

Jump to bottom

Consolidate/refactor local exceptions in C# #2338

Merged
merged 22 commits into from
Jun 25, 2024
Merged

Consolidate/refactor local exceptions in C# #2338

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
686fd4a
Initial local exception refactoring
bernardnormier Jun 22, 2024
aa1cba4
Checkpoint
bernardnormier Jun 22, 2024
e1df52c
Remove EndpointParseException
bernardnormier Jun 22, 2024
31fc9b3
Remove ProxyParseException
bernardnormier Jun 22, 2024
860465f
Remove remaining XxxParseException
bernardnormier Jun 22, 2024
df9c5f6
Remove Illegal exceptions
bernardnormier Jun 22, 2024
875fe1b
Fix ProtocolException
bernardnormier Jun 23, 2024
822d2db
Removed exceptions derived from MarshalException
bernardnormier Jun 23, 2024
a32b121
Move more exceptions
bernardnormier Jun 23, 2024
edd5586
Checkpoint
bernardnormier Jun 23, 2024
d48a9da
Cleanup
bernardnormier Jun 23, 2024
8c12111
More cleanup
bernardnormier Jun 23, 2024
004157d
Fix IceBox local exception
bernardnormier Jun 23, 2024
f79da3c
More cleanup
bernardnormier Jun 23, 2024
3b5fc1b
Add ConnectionAbortedException
bernardnormier Jun 23, 2024
fd4e492
Fix RFE
bernardnormier Jun 23, 2024
e161841
Reformat
bernardnormier Jun 24, 2024
2b66086
Update csharp/src/IceBox/Service.cs
bernardnormier Jun 24, 2024
316bb73
Fix review comment + cross-test failures
bernardnormier Jun 24, 2024
d89278e
Merge remote-tracking branch 'origin/main' into cs-local
bernardnormier Jun 25, 2024
29b026d
Fix cross-test
bernardnormier Jun 25, 2024
28cdd3c
Remove bogus innerException parameter
bernardnormier Jun 25, 2024
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
31 changes: 31 additions & 0 deletions CHANGELOG-3.8.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,37 @@ These are the changes since the Ice 3.7.10 release in [CHANGELOG-3.7.md](./CHANG
(the unit is seconds). You can also override this value for a specific object adapter with the configuration
property `AdapterName.Connection.CloseTimeout`.

- Consolidate and refactor the exceptions derived from LocalException.
| Local exception in Ice 3.7 | Replacement |
|-------------------------------------|----------------------------|
| BadMagicException | ProtocolException (base) |
Copy link
Member Author

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.

| CompressionException | ProtocolException (base) |
| ConnectionManuallyClosedException | ConnectionAbortedException, ConnectionClosedException |
| ConnectionNotValidatedException | ProtocolException (base) |
| EncapsulationException | MarshalException (base) |
| EndpointParseException | ParseException |
| EndpointSelectionTypeParseException | ParseException |
| IllegalIdentityException | ArgumentException (C#) |
| IllegalMessageSizeException | MarshalException |
| IllegalServantException | ArgumentNullException (C#) |
| IdentityParseException | ParseException |
| MemoryLimitException | MarshalException (base) |
| NoValueFactoryException | MarshalException (base) |
| ProxyParseException | ParseException |
| ProxyUnmarshalException | MarshalException (base) |
| UnexpectedObjectException | MarshalException (base) |
| UnknownMessageException | ProtocolException (base) |
| UnknownReplyStatusException | MarshalException |
| UnmarshalOutOfBoundsException | MarshalException (base) |
| UnsupportedEncodingException | MarshalException |
| UnsupportedProtocolException | MarshalException, FeatureNotSupportedException |
| VersionParseException | ParseException |

base = was existing base class

New local exceptions:\
ConnectionAbortedException, ConnectionClosedException, ConnectionIdleException, ParseException

## Slice Language Changes

- Removed support for the `["preserve-slice"]` metadata. Slice classes encoded in the Sliced-format are now always
Expand Down
5 changes: 2 additions & 3 deletions csharp/src/Ice/Communicator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,8 @@ public bool isShutdown()
/// Converts a stringified proxy into a proxy.
/// For example, "MyCategory/MyObject:tcp -h some_host -p 10000" creates a proxy that refers to the Ice object
/// having an identity with a name "MyObject" and a category "MyCategory", with the server running on host
/// "some_host", port 10000. If the stringified proxy does not parse correctly, this method throws one of
/// ProxyParseException, EndpointParseException, or IdentityParseException. Refer to the Ice manual for a detailed
/// description of the syntax supported by stringified proxies.
/// "some_host", port 10000. If the stringified proxy does not parse correctly, this method throws ParseException.
/// Refer to the Ice manual for a detailed description of the syntax supported by stringified proxies.
/// </summary>
/// <param name="str">The stringified proxy to convert into a proxy.</param>
/// <returns>The proxy, or null if str is an empty string.</returns>
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Ice/Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ System.Threading.Tasks.Task heartbeatAsync(
/// Throw an exception indicating the reason for connection closure.
/// For example,
/// CloseConnectionException is raised if the connection was closed gracefully, whereas
/// ConnectionManuallyClosedException is raised if the connection was manually closed by
/// ConnectionAbortedException/ConnectionClosedException is raised if the connection was manually closed by
/// the application. This operation does nothing if the connection is not yet closed.
/// </summary>
void throwException();
Expand Down
106 changes: 66 additions & 40 deletions csharp/src/Ice/ConnectionI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ internal void destroy(int reason)
{
case ObjectAdapterDeactivated:
{
setState(StateClosing, new ObjectAdapterDeactivatedException());
setState(StateClosing, new ObjectAdapterDeactivatedException(_adapter?.getName() ?? ""));
break;
}

Expand All @@ -161,11 +161,18 @@ public void close(ConnectionClose mode)
{
if (mode == ConnectionClose.Forcefully)
{
setState(StateClosed, new ConnectionManuallyClosedException(false));
setState(StateClosed,
new ConnectionAbortedException(
"Connection close forcefully by the application.",
closedByApplication: true));
Copy link
Member

Choose a reason for hiding this comment

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

Is closeByApplication ever false?

Copy link
Member Author

@bernardnormier bernardnormier Jun 24, 2024 •
edited
Loading

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.

}
else if (mode == ConnectionClose.Gracefully)
{
setState(StateClosing, new ConnectionManuallyClosedException(true));
setState(
StateClosing,
new ConnectionClosedException(
"Connection close gracefully by the application.",
closedByApplication: true));
}
else
{
Expand All @@ -179,7 +186,11 @@ public void close(ConnectionClose mode)
Monitor.Wait(this);
}

setState(StateClosing, new ConnectionManuallyClosedException(true));
setState(
StateClosing,
new ConnectionClosedException(
"Connection close gracefully by the application.",
closedByApplication: true));
}
}
}
Expand Down Expand Up @@ -884,7 +895,7 @@ public override void message(ref ThreadPoolCurrent current)
//
// This situation is possible for small UDP packets.
//
throw new IllegalMessageSizeException();
throw new MarshalException("Received Ice message with too few bytes in header.");
}

// Decode the header.
Expand All @@ -897,22 +908,29 @@ public override void message(ref ThreadPoolCurrent current)
if (m[0] != Protocol.magic[0] || m[1] != Protocol.magic[1] ||
m[2] != Protocol.magic[2] || m[3] != Protocol.magic[3])
{
BadMagicException ex = new BadMagicException();
ex.badMagic = m;
throw ex;
throw new ProtocolException(
$"Bad magic in message header: {m[0]:X2} {m[1]:X2} {m[2]:X2} {m[3]:X2}");
}

ProtocolVersion pv = new ProtocolVersion(_readStream);
Protocol.checkSupportedProtocol(pv);
EncodingVersion ev = new EncodingVersion(_readStream);
Protocol.checkSupportedProtocolEncoding(ev);
var pv = new ProtocolVersion(_readStream);
if (pv != Util.currentProtocol)
{
throw new MarshalException(
$"Invalid protocol version in message header: {pv.major}.{pv.minor}");
}
var ev = new EncodingVersion(_readStream);
if (ev != Util.currentProtocolEncoding)
{
throw new MarshalException(
$"Invalid protocol encoding version in message header: {ev.major}.{ev.minor}");
}

_readStream.readByte(); // messageType
_readStream.readByte(); // compress
int size = _readStream.readInt();
if (size < Protocol.headerSize)
{
throw new IllegalMessageSizeException();
throw new MarshalException($"Received Ice message with unexpected size {size}.");
}

// Resize the read buffer to the message size.
Expand Down Expand Up @@ -1249,7 +1267,7 @@ private void finish()
// Trace the cause of unexpected connection closures
//
if (!(_exception is CloseConnectionException ||
_exception is ConnectionManuallyClosedException ||
_exception is ConnectionAbortedException ||
_exception is ConnectionClosedException ||
_exception is ConnectionIdleException ||
_exception is CommunicatorDestroyedException ||
Expand Down Expand Up @@ -1507,16 +1525,16 @@ internal void idleCheck(TimeSpan idleTimeout)
{
if (isActiveOrHolding())
{
int idleTimeoutInSeconds = (int)idleTimeout.TotalSeconds;

if (_instance.traceLevels().network >= 1)
{
int idleTimeoutInSeconds = (int)idleTimeout.TotalSeconds;

_instance.initializationData().logger.trace(
_instance.traceLevels().networkCat,
$"connection aborted by the idle check because it did not receive any bytes for {idleTimeoutInSeconds}s\n{_transceiver.toDetailedString()}");
}

setState(StateClosed, new ConnectionIdleException());
setState(StateClosed, new ConnectionIdleException($"Connection aborted by the idle check because it did not receive any bytes for {idleTimeoutInSeconds}s."));
}
// else nothing to do
}
Expand Down Expand Up @@ -1590,7 +1608,7 @@ private void setState(int state, LocalException ex)
// Don't warn about certain expected exceptions.
//
if (!(_exception is CloseConnectionException ||
_exception is ConnectionManuallyClosedException ||
_exception is ConnectionAbortedException ||
_exception is ConnectionClosedException ||
_exception is ConnectionIdleException ||
_exception is CommunicatorDestroyedException ||
Expand Down Expand Up @@ -1753,7 +1771,7 @@ private void setState(int state)
if (_observer is not null && state == StateClosed && _exception is not null)
{
if (!(_exception is CloseConnectionException ||
_exception is ConnectionManuallyClosedException ||
_exception is ConnectionAbortedException ||
_exception is ConnectionClosedException ||
_exception is ConnectionIdleException ||
_exception is CommunicatorDestroyedException ||
Expand Down Expand Up @@ -1921,27 +1939,34 @@ private bool validate(int operation)
if (m[0] != Protocol.magic[0] || m[1] != Protocol.magic[1] ||
m[2] != Protocol.magic[2] || m[3] != Protocol.magic[3])
{
BadMagicException ex = new BadMagicException();
ex.badMagic = m;
throw ex;
throw new ProtocolException(
$"Bad magic in message header: {m[0]:X2} {m[1]:X2} {m[2]:X2} {m[3]:X2}");
}

ProtocolVersion pv = new ProtocolVersion(_readStream);
Protocol.checkSupportedProtocol(pv);

EncodingVersion ev = new EncodingVersion(_readStream);
Protocol.checkSupportedProtocolEncoding(ev);
var pv = new ProtocolVersion(_readStream);
if (pv != Util.currentProtocol)
{
throw new MarshalException(
$"Invalid protocol version in message header: {pv.major}.{pv.minor}");
}
var ev = new EncodingVersion(_readStream);
if (ev != Util.currentProtocolEncoding)
{
throw new MarshalException(
$"Invalid protocol encoding version in message header: {ev.major}.{ev.minor}");
}

byte messageType = _readStream.readByte();
if (messageType != Protocol.validateConnectionMsg)
{
throw new ConnectionNotValidatedException();
throw new ProtocolException(
$"Received message of type {messageType} on connection that is not yet validated.");
}
_readStream.readByte(); // Ignore compression status for validate connection.
int size = _readStream.readInt();
if (size != Protocol.headerSize)
{
throw new IllegalMessageSizeException();
throw new MarshalException($"Received ValidateConnection message with unexpected size {size}.");
}
TraceUtil.traceRecv(_readStream, _logger, _traceLevels);
}
Expand Down Expand Up @@ -2283,9 +2308,7 @@ private int parseMessage(ref MessageInfo info)
else
{
string lib = AssemblyUtil.isWindows ? "bzip2.dll" : "libbz2.so.1";
FeatureNotSupportedException ex = new FeatureNotSupportedException();
ex.unsupportedFeature = "Cannot uncompress compressed message: " + lib + " not found";
throw ex;
throw new FeatureNotSupportedException($"Cannot uncompress compressed message: {lib} not found");
}
}
info.stream.pos(Protocol.headerSize);
Expand Down Expand Up @@ -2359,12 +2382,12 @@ private int parseMessage(ref MessageInfo info)
else
{
TraceUtil.traceRecv(info.stream, _logger, _traceLevels);
info.requestCount = info.stream.readInt();
if (info.requestCount < 0)
int requestCount = info.stream.readInt();
if (requestCount < 0)
{
info.requestCount = 0;
throw new UnmarshalOutOfBoundsException();
throw new MarshalException($"Received batch request with {requestCount} batches.");
}
info.requestCount = requestCount;
info.adapter = _adapter;
info.upcallCount += info.requestCount;

Expand Down Expand Up @@ -2422,7 +2445,8 @@ private int parseMessage(ref MessageInfo info)
{
TraceUtil.trace("received unknown message\n(invalid, closing connection)",
info.stream, _logger, _traceLevels);
throw new UnknownMessageException();

throw new ProtocolException($"Received Ice protocol message with unknown type: {messageType}");
}
}
}
Expand Down Expand Up @@ -2613,9 +2637,11 @@ private void inactivityCheck(System.Threading.Timer inactivityTimer)

if (_state == StateActive)
{
// TODO: fix LocalException to accept a message
// "connection closed because it remained inactive for longer than the inactivity timeout"
setState(StateClosing, new ConnectionClosedException());
setState(
StateClosing,
new ConnectionClosedException(
"Connection closed because it remained inactive for longer than the inactivity timeout.",
closedByApplication: false));
}
}
// Else this timer was already canceled and disposed. Nothing to do.
Expand Down
29 changes: 11 additions & 18 deletions csharp/src/Ice/CurrentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,39 +157,32 @@ OutgoingResponse createOutgoingResponseCore(System.Exception exc)
_ => throw new Ice.MarshalException("Unexpected exception type")
};

if (rfe.id.name.Length == 0)
Identity id = rfe.id;
string facet = rfe.facet;
if (id.name.Length == 0)
{
rfe.id = current.id;
id = current.id;
facet = current.facet;
}
string operation = rfe.operation.Length == 0 ? current.operation : rfe.operation;

if (rfe.facet.Length == 0 && current.facet.Length > 0)
{
rfe.facet = current.facet;
}

if (rfe.operation.Length == 0 && current.operation.Length > 0)
{
rfe.operation = current.operation;
}

// Called after fixing id, facet and operation.
exceptionMessage = rfe.ToString();
exceptionMessage = RequestFailedException.createMessage(rfe.GetType().Name, id, facet, operation);

if (current.requestId != 0)
{
ostr.writeByte((byte)replyStatus);
Identity.ice_write(ostr, rfe.id);
Identity.ice_write(ostr, id);

if (rfe.facet.Length == 0)
if (facet.Length == 0)
{
ostr.writeStringSeq([]);
}
else
{
ostr.writeStringSeq([rfe.facet]);
ostr.writeStringSeq([facet]);
}

ostr.writeString(rfe.operation);
ostr.writeString(operation);
}
break;

Expand Down
Loading
Loading