Bug 1627841 - Cancel on exceptions in wrapped measurement function #808
Conversation
badboy
force-pushed
the
1627841/measure-but-throw
branch
from
50b55f0 to
c8cccaf
Compare
| funcToMeasure() | ||
| } catch (e: Exception) { | ||
| cancel() | ||
| throw e |
There was a problem hiding this comment.
I don't think we should throw exceptions from our APIs. We never do, our APIs are execption safe. We should instead document that we swallow exception and if a different behaviour is required then the product should not use this method.
There was a problem hiding this comment.
Hm, I kinda disagree with that. We're handling user-provided functions (for convenience) and we're merely passing on the behavior of their code (it's not that different from returning their return value).
There was a problem hiding this comment.
This would be the only API function behaving different, which is something I don't think we should do (... that would be an exception ). We can handle user-defined function, but it doesn't mean it should break consistency with our other APIs.
This doesn't block any other use-case, since the other APIs can still be used.
There was a problem hiding this comment.
IMO it doesn't behave "differently". Glean itself still doesn't produce any exceptions (I see what you did there!).
If users have potentially-throwing functions they need to measure they again need to come up with the exact same wrappper I wrote here instead of using our measure utility.
There was a problem hiding this comment.
Functions with exceptions can still be measured. To me this behaves differently: we guarantee that our APIs do not throw exceptions. Having one API suddenly behaving differently (even if the exception originates from a user defined function) can be confusing and a source of subtle bugs. Consumers of the API have two (with no exception) have 3 choices:
(1) Catch any exception in the function they need to measure.
(2) Live with the fact that our APIs swallow exceptions.
(3) Not use this utility wrapper.
I'd rather have consistency with the fact that users can expect no exception to come through our APIs than this added flexibility
There was a problem hiding this comment.
@travis79 I do like the idea of allowing choosing a behaviour, but I think we should always cancel on exception and only allow to choose whether or not to propagate the exception :)
As long as we default to propagating the exception since that would be the behavior if the function weren't being measured, I am okay with making that configurable within the function. As for whether or not to cancel? My opinion on that is weakly held as I don't have a concrete use case for not cancelling, so I am fine with the cancel on exception behavior also.
There was a problem hiding this comment.
As long as we default to propagating the exception since that would be the behavior if the function weren't being measured
I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)
There was a problem hiding this comment.
I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)
The only problem is that we would be swallowing application exceptions and not Glean exceptions, and I'm not comfortable with that. If a consuming application expected a function to throw in certain cases and we swallow that inside of the measure function, then we are preventing the application from handling its own exceptions. I totally agree that Glean shouldn't cause exceptions in the application, but we shouldn't interfere with an exception raised by a function defined in the application.
There was a problem hiding this comment.
I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)
The only problem is that we would be swallowing application exceptions and not Glean exceptions, and I'm not comfortable with that. If a consuming application expected a function to throw in certain cases and we swallow that inside of the measure function, then we are preventing the application from handling its own exceptions.
All right, while I don't agree with this default behaviour, your position is relatively similar to @badboy 's one.
So I'd be fine with taking the initial solution by @badboy with documentation explicitly stating that this might throw if the function throws.
There was a problem hiding this comment.
I realize that I forgot the docs on this function, but there is a sentence on the other function. Will adopt that and then land it.
Swift is more explicit: fallible functions have to be annotated as such (`throws`). Everything else can't throw. If someone wants to measure a fallible function (but get the same cancellation of the timing when thrown) we need another function. Luckily we can overload functions, so we can keep the same name.
badboy
force-pushed
the
1627841/measure-but-throw
branch
from
c8cccaf to
6497c09
Compare
Needs iOS tests before merge.