search

Azure / azure-libraries-for-java

Azure Management Libraries for Java
https://docs.microsoft.com/en-us/java/azure/
MIT License
94 stars 97 forks source link

Improving Exceptions bubbling #1397

Open blacelle opened 3 years ago

blacelle commented 3 years ago

Is your feature request related to a problem? Please describe. In many places, any sort of exceptions is bubbled without a context, leaving the stack-trace reader with a partial stack difficult to process

Describe the solution you'd like I would like Exceptions to be properly chained, so that finally reported Exceptions holds a proper chain of Cause, enabling retracing the whole chain of calls.

Sorry for the quite long ticket while I suspect it may be rejected right-away, I tried to be quite exhaustive in my analysis.

Typically, I consider a piece of code like:

com.microsoft.azure.management.Azure azure = ...;

azure .resourceGroups().list()

I end with a failure in ResourceGroupsInner:

    public Observable<ServiceResponse<Page<ResourceGroupInner>>> listSinglePageAsync() {
        if (this.client.subscriptionId() == null) {
            throw new IllegalArgumentException("Parameter this.client.subscriptionId() is required and cannot be null.");
        }
        if (this.client.apiVersion() == null) {
            throw new IllegalArgumentException("Parameter this.client.apiVersion() is required and cannot be null.");
        }
        final String filter = null;
        final Integer top = null;
        return service.list(this.client.subscriptionId(), filter, top, this.client.apiVersion(), this.client.acceptLanguage(), this.client.userAgent())
            .flatMap(new Func1<Response<ResponseBody>, Observable<ServiceResponse<Page<ResourceGroupInner>>>>() {
                @Override
                public Observable<ServiceResponse<Page<ResourceGroupInner>>> call(Response<ResponseBody> response) {
                    try {
                        ServiceResponse<PageImpl<ResourceGroupInner>> result = listDelegate(response);
                        return Observable.just(new ServiceResponse<Page<ResourceGroupInner>>(result.body(), result.response()));
                    } catch (Throwable t) {
                        return Observable.error(t);
                    }
                }
            });
    }

This leads to stacks like:

2021-05-11 12:33:19.259 ERROR 2928 --- [nio-3000-exec-9] MyCustomExceptionHandler      : Runtime error : Status code 403, {"error":{"code":"AuthorizationFailed","message":"The client 'XXX with object id 'XXX' does not have authorization to perform action 'Microsoft.Resources/subscriptions/resourcegroups/read' over scope '/subscriptions/YYY' or the scope is invalid. If access was recently granted, please refresh your credentials."}} 

com.microsoft.azure.CloudException: Status code 403, {"error":{"code":"AuthorizationFailed","message":"The client 'XXX' with object id 'XXX' does not have authorization to perform action 'Microsoft.Resources/subscriptions/resourcegroups/read' over scope '/subscriptions/YYY' or the scope is invalid. If access was recently granted, please refresh your credentials."}}
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
    at com.microsoft.rest.ServiceResponseBuilder.build(ServiceResponseBuilder.java:122)
    at com.microsoft.azure.AzureResponseBuilder.build(AzureResponseBuilder.java:56)
    at com.microsoft.azure.management.resources.implementation.ResourceGroupsInner.listDelegate(ResourceGroupsInner.java:973)
    at com.microsoft.azure.management.resources.implementation.ResourceGroupsInner.access$600(ResourceGroupsInner.java:49)
    at com.microsoft.azure.management.resources.implementation.ResourceGroupsInner$31.call(ResourceGroupsInner.java:850)
    at com.microsoft.azure.management.resources.implementation.ResourceGroupsInner$31.call(ResourceGroupsInner.java:846)
    at rx.internal.operators.OnSubscribeMap$MapSubscriber.onNext(OnSubscribeMap.java:69)
    at retrofit2.adapter.rxjava.CallArbiter.deliverResponse(CallArbiter.java:120)
    at retrofit2.adapter.rxjava.CallArbiter.emitResponse(CallArbiter.java:102)
    at retrofit2.adapter.rxjava.CallEnqueueOnSubscribe$1.onResponse(CallEnqueueOnSubscribe.java:41)
    at retrofit2.OkHttpCall$1.onResponse(OkHttpCall.java:129)
    at okhttp3.RealCall$AsyncCall.execute(RealCall.java:174)
    at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
    at java.lang.Thread.run(Thread.java:748)

My main issue here is that I capture an Exception is some sort of generic Exception handler (e.g. a Spring @ControllerAdvice). However, the stack does not even report my class having called azure .resourceGroups().list().

A richer stack would be available by wrapping the bubble-ed exception in ResourceGroupsInner.listSinglePageAsync :

return Observable.error(new RuntimeException(t));

However, this would not produce a full stack due to :

rx.exceptions.Exceptions.propagate(Throwable) used by BlockingObservable:

In my case, the stack looks like:

Daemon Thread [http-nio-3000-exec-1] (Suspended (exception CloudException)) 
    owns: NioEndpoint$NioSocketWrapper  (id=432)    
    Exceptions.propagate(Throwable) line: 53    
    BlockingObservable<T>.blockForSingle(Observable<? extends T>) line: 463 
    BlockingObservable<T>.single() line: 340    
    ResourceGroupsInner.list() line: 766    
    ResourceGroupsImpl.list() line: 41  
        .....

Hence, the calling thread own stack is not properly bubbled due to:

     public static RuntimeException propagate(Throwable t) {
    if (t instanceof RuntimeException) {
        throw (RuntimeException) t;
    } else if (t instanceof Error) {
        throw (Error) t;
    } else {
        throw new RuntimeException(t); // NOPMD
    }
}

The questions of exception bubbling in Rxjava is regularly considered (e.g. https://github.com/ReactiveX/RxJava/issues/969).

However, blocking statements are considered inappropriate for production:

BlockingObservable: 

It can be
 * useful for testing and demo purposes, but is generally inappropriate for production applications (if you
 * think you need to use a {@code BlockingObservable} this is usually a sign that you should rethink your
 * design).

Should I consider this as an issue in Rxjava BlockingObservable ? (i.e. BlockingObservable does not propagate properly exceptions from different threads, hence losing the stack for calling thread?)

Or should I consider this should be managed by calling libraries (here Azure libraries), to catch systematically such exceptions, and re-wrap them? However, this would lead to the inner exception to be be available directly (still being available through the chain of causes).

In the second case, it would lead in any sync call to something like:

ResourceGroupsInner:

public PagedList<ResourceGroupInner> list() {
    try {
        ServiceResponse<Page<ResourceGroupInner>> response = listSinglePageAsync().toBlocking().single();
    } catch (RuntimeException e) {
        throw new RuntimeException("Ouch", e);
    }
    return new PagedList<ResourceGroupInner>(response.body()) {
        @Override
        public Page<ResourceGroupInner> nextPage(String nextPageLink) {
            return listNextSinglePageAsync(nextPageLink).toBlocking().single().body();
        }
    };
}
weidongxu-microsoft commented 3 years ago

This is a valid issue. However we are not likely to do a large change to the repo at this point (which requires discussing and finding a somewhat better solution, rewrite the code generator, regenerate all the underlying code (all those FooInners)).

Develop focus is shifted to next generate SDK https://aka.ms/azsdk/java/mgmt (migration should be easy, though PagedList is replaced by PagedIterable), which now uses reactor as underlying async lib. However the sync/async is designed similarly, as sync being a blocked wrapper over async operation.

A similar call of azure.resourceGroups().list().stream().count() (note PagedIterable will no longer send request if no element is consumed) give me this stack. At least this one gives me the location of the code (i.e. ManageResource.runSample(ManageResource.java:40)).

com.azure.core.management.exception.ManagementException: ...
    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:64)
    at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
    at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
    at com.azure.core.http.rest.RestProxy.instantiateUnexpectedException(RestProxy.java:343)
    at com.azure.core.http.rest.RestProxy.lambda$ensureExpectedStatus$5(RestProxy.java:382)
    ...
    at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
    at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
    at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
    at java.base/java.lang.Thread.run(Thread.java:832)
    Suppressed: java.lang.Exception: #block terminated with an error
        at reactor.core.publisher.BlockingSingleSubscriber.blockingGet(BlockingSingleSubscriber.java:99)
        at reactor.core.publisher.Flux.blockLast(Flux.java:2519)
        at com.azure.core.util.paging.ContinuablePagedByIteratorBase.requestPage(ContinuablePagedByIteratorBase.java:94)
        at com.azure.core.util.paging.ContinuablePagedByItemIterable$ContinuablePagedByItemIterator.<init>(ContinuablePagedByItemIterable.java:50)
        at com.azure.core.util.paging.ContinuablePagedByItemIterable.iterator(ContinuablePagedByItemIterable.java:37)
        at java.base/java.lang.Iterable.spliterator(Iterable.java:101)
        at com.azure.core.util.paging.ContinuablePagedIterable.stream(ContinuablePagedIterable.java:50)
        at com.azure.resourcemanager.resources.fluentcore.utils.PagedConverter$PagedIterableImpl.stream(PagedConverter.java:200)
        at com.azure.resourcemanager.resources.samples.ManageResource.runSample(ManageResource.java:40)
        at com.azure.resourcemanager.samples.ResourceSampleTests.testManageResource(ResourceSampleTests.java:56)
blacelle commented 3 years ago

Thanks @weidongxu-microsoft. In fact, I discovered this alternate Azure Management library very recently. The README for this repository indicates there is a more recent alternative. But it was not crystal-clear to myself. I interprete Try the Next-Generation Azure Management SDK for Java now that current current repo is the next-generation, and I did not read the rest of the paragraph (too bad).

Would you mind making the point this library is kind of deprecated clearer? Deprecation is maybe too strong for now, but you get my point.

weidongxu-microsoft commented 3 years ago

@blacelle

Well, it is not deprecated (yet). It is kind of in this state: if we have bug or improvement related to security, we fix here and on the new repo; if we have new feature or other improvement, we likely only put it in the new repo.