[ 
https://issues.apache.org/jira/browse/SCB-580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16477044#comment-16477044
 ] 

ASF GitHub Bot commented on SCB-580:
------------------------------------

wujimin commented on a change in pull request #702: [SCB-580] fix response of 
the upload file too large
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/702#discussion_r188543532
 
 

 ##########
 File path: 
transports/transport-rest/transport-rest-vertx/src/main/java/org/apache/servicecomb/transport/rest/vertx/VertxRestDispatcher.java
 ##########
 @@ -64,9 +70,106 @@ private void failureHandler(RoutingContext context) {
         e = cause;
       }
     }
-    restProducerInvocation.sendFailResponse(e);
 
-    // 走到这里,应该都是不可控制的异常,直接关闭连接
+    // only when unexpected exception happens, it will run into here.
+    // the connection should be closed.
+    handleFailureAndClose(context, restProducerInvocation, e);
+  }
+
+  /**
+   * Try to find out the failure information and send it in response.
+   */
+  private void handleFailureAndClose(RoutingContext context, 
AbstractRestInvocation restProducerInvocation,
+      Throwable e) {
+    if (null != restProducerInvocation) {
+      // if there is restProducerInvocation, let it send exception in 
response. The exception is allowed to be null.
+      sendFailResponseByInvocation(context, restProducerInvocation, e);
+      return;
+    }
+
+    if (null != e) {
+      // if there exists exception, try to send this exception by 
RoutingContext
+      sendExceptionByRoutingContext(context, e);
+      return;
+    }
+
+    // if there is no exception, the response is determined by status code.
+    sendFailureRespDeterminedByStatus(context);
+  }
+
+  /**
+   * Try to determine response by status code, and send response.
+   */
+  private void sendFailureRespDeterminedByStatus(RoutingContext context) {
+    Family statusFamily = Family.familyOf(context.statusCode());
+    if (Family.CLIENT_ERROR.equals(statusFamily) || 
Family.SERVER_ERROR.equals(statusFamily) || Family.OTHER
+        .equals(statusFamily)) {
+      context.response().putHeader(HttpHeaders.CONTENT_TYPE, 
MediaType.WILDCARD)
+          .setStatusCode(context.statusCode()).end();
+    } else {
+      // it seems the status code is not set properly
+      context.response().putHeader(HttpHeaders.CONTENT_TYPE, 
MediaType.WILDCARD)
+          .setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode())
+          .setStatusMessage(Status.INTERNAL_SERVER_ERROR.getReasonPhrase())
+          
.end(wrapResponseBody(Status.INTERNAL_SERVER_ERROR.getReasonPhrase()));
+    }
+    context.response().close();
+  }
+
+  /**
+   * Use routingContext to send failure information in throwable.
+   */
+  private void sendExceptionByRoutingContext(RoutingContext context, Throwable 
e) {
+    if (InvocationException.class.isInstance(e)) {
+      InvocationException invocationException = (InvocationException) e;
+      context.response().putHeader(HttpHeaders.CONTENT_TYPE, 
MediaType.WILDCARD)
+          
.setStatusCode(invocationException.getStatusCode()).setStatusMessage(invocationException.getReasonPhrase())
+          .end(wrapResponseBody(invocationException.getReasonPhrase()));
+    } else {
+      context.response().putHeader(HttpHeaders.CONTENT_TYPE, 
MediaType.WILDCARD)
+          
.setStatusCode(Status.INTERNAL_SERVER_ERROR.getStatusCode()).end(wrapResponseBody(e.getMessage()));
+    }
+    context.response().close();
+  }
+
+  /**
+   * Consumer will treat the response body as json by default, so it's 
necessary to wrap response body as Json string
+   * to avoid deserialization error.
+   *
+   * @param message response body
+   * @return response body wrapped as Json string
+   */
+  String wrapResponseBody(String message) {
+    if (isValidJson(message)) {
+      return message;
+    }
+
+    JsonObject jsonObject = new JsonObject();
+    jsonObject.put("message", message);
+
+    return jsonObject.toString();
 
 Review comment:
   this will produce:
   {
     "message": ......
   }
   
   is this we expected?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> When upload file size exceeds limitation of provider, consumer will return a 
> confusing response
> -----------------------------------------------------------------------------------------------
>
>                 Key: SCB-580
>                 URL: https://issues.apache.org/jira/browse/SCB-580
>             Project: Apache ServiceComb
>          Issue Type: Bug
>          Components: Java-Chassis
>            Reporter: YaoHaishi
>            Assignee: YaoHaishi
>            Priority: Major
>             Fix For: java-chassis-1.0.0-m2
>
>         Attachments: compile_warning.PNG, consumerResponse.PNG, 
> providerExceptionLog.PNG
>
>
> Test based on 1.0.0.B009
> Let user upload file to consumer, and then consumer upload file to provider. 
> If the file size is under the limitation of consumer but exceeds the 
> limitation of provider, the provider will throw a NullPointerException, but 
> the consumer will response "method POST, path /upload/multipartFileUpload/, 
> statusCode 500, reasonPhrase Internal Server Error, response content-type 
> null is not supported".(In the log of consumer, no error is printed)
>  provider log:
>  !providerExceptionLog.PNG! 
>  consumer response:
>  !consumerResponse.PNG!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to