search

zeroc-ice / ice

All-in-one solution for creating networked applications with RPC, pub/sub, server deployment, and more.
https://zeroc.com
GNU General Public License v2.0
2k stars 592 forks source link

Consider Forwarding `FacetNotExistException` through `checkedCast`s #2323

Open InsertCreativityHere opened 1 week ago

InsertCreativityHere commented 1 week ago

Currently, in Java, calling checkedCast will always eat FacetNotExistException, and if caught, we return null: https://github.com/zeroc-ice/ice/blob/b81c856d81add8691dafe56ac51097faed13a9da/java/src/Ice/src/main/java/com/zeroc/Ice/ObjectPrx.java#L879-L880 So it's impossible for checkedCast to ever throw this exception, in Java.

But in C# and C++, we only eat this exception if you passed in a facet parameter. Because we have 2 separate implementations, one which accepts a facet and one that doesn't. (In java, we do have separate overloads, but they all delegate to the single implementation linked above)

At the least, this is inconsistent, so it'd be good to pick one behavior, and apply it to all our language mappings. So, I propose that we completely drop this behavior of eating FacetNotException, and should just always rethrow it.

A) This is the simplest option, and lets us avoid this try-catch-return-null wrapper block. B) Is anyone relying on this behavior of FacetNotExist getting mapped to a null proxy? I'd suspect this is so niche that no one is. C) It will make C++ and C# cleaner, because then they (like Java) would only need a single implementation.

To me, it just feels weird that we're eating this exception at all. If your facet doesn't exist... wouldn't you want some way of knowing?

bernardnormier commented 1 week ago

I am ok with changing this behavior and letting FacetNotExistException through.

Note however that this is a thoroughly documented and tested behavior: https://doc.zeroc.com/ice/3.7/client-server-features/facets

If an Ice object does not provide the specified facet, checkedCast returns null:

Note that checkedCast also returns a null proxy if a facet exists, but the cast is to the wrong type. For example:

pepone commented 1 week ago

The C# doc comment is different:

/// Creates a new proxy that is identical to the passed proxy, except
/// for its facet. This call contacts
/// the server and throws an Ice run-time exception if the target
/// object does not exist, the specified facet does not exist, or the server cannot be reached.

JavaScript does like C# and C++, I think we should just fix Java to conform with the documented behavior.

bernardnormier commented 1 week ago

This C# doc-comment does not match the C# implementation and does not match the documented behavior in https://doc.zeroc.com/ice/3.7/client-server-features/facets.

I think we should either: a) keep the documented behavior and fix the implementation / doc-comments that don't follow this documented behavior

This means the checkedCast-with-facet returns null when ice_isA throws FacetNotExistException, while the checkedCast without facet does not handle any exception thrown by ice_isA

or

b) change the behavior as Austin proposed and fix all documentation / doc-comments / tests

The proposed changed behavior is for all checkedCast to not catch any exception thrown by ice_isA.

bernardnormier commented 1 week ago

In theory, there is a third option:

c) change the checkedCast behavior to always catch FacetNotException and return null for FacetNotException

That's what we currently do in Java.

Pros: like (b), all checkedCast are implemented the same. Cons: it's semantically weird. FacetNotExist => null but ObjectNotExist => exception?

So I don't think we should consider this option.