From b6ec55f1ccf26972faee887ec49733bad0768cff Mon Sep 17 00:00:00 2001 From: Iva Avramova Date: Mon, 11 Mar 2024 10:00:04 +0200 Subject: [PATCH 1/5] Fix of exception handler trying to write response to a disconnected client Why: On Conductor server side, an exception gets raised `Failure in @ExceptionHandler com.netflix.conductor.rest.controllers.ApplicationExceptionMapper#handleAll(HttpServletRequest, Throwable)`. Cause is attempting to write while the client aborted the connection `org.apache.catalina.connector.ClientAbortException: java.io.IOException: Broken pipe`, while the exception was wrapped in `ClosedChannelException`. What: Added a ClosedChannelException handler for such exceptions root caused by ClientAbortException. Testing done: Local run. Not trivial to emulate the issue in a test. Such a test may rely on timing out the client before server responded. Signed-off-by: Iva Avramova --- .../ApplicationExceptionMapper.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java index 505bf9678..251144c81 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java @@ -12,10 +12,12 @@ */ package com.netflix.conductor.rest.controllers; +import java.nio.channels.ClosedChannelException; import java.util.HashMap; import java.util.Map; import org.apache.catalina.connector.ClientAbortException; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.core.annotation.Order; @@ -53,8 +55,21 @@ public class ApplicationExceptionMapper { EXCEPTION_STATUS_MAP.put(NoResourceFoundException.class, HttpStatus.NOT_FOUND); } - @ExceptionHandler(ClientAbortException.class) + @ExceptionHandler(ClosedChannelException.class) @Order(ValidationExceptionMapper.ORDER + 1) + public void handleNestedClientAbortedInClosedChannelException( + HttpServletRequest request, ClosedChannelException closedChannelException) { + final Throwable rootCause = ExceptionUtils.getRootCause(closedChannelException); + if (rootCause != null + && ClientAbortException.class.getName().equals(rootCause.getClass().getName())) { + handleClientAborted(request, (ClientAbortException) rootCause); + return; + } + handleAll(request, closedChannelException); + } + + @ExceptionHandler(ClientAbortException.class) + @Order(ValidationExceptionMapper.ORDER + 2) public void handleClientAborted( HttpServletRequest request, ClientAbortException clientAbortException) { logException( @@ -62,7 +77,7 @@ public void handleClientAborted( } @ExceptionHandler(Throwable.class) - @Order(ValidationExceptionMapper.ORDER + 2) + @Order(ValidationExceptionMapper.ORDER + 3) public ResponseEntity handleAll(HttpServletRequest request, Throwable th) { logException(request, th); From f1a1afeadf7a1415beb1469c2421ae27dee2c19f Mon Sep 17 00:00:00 2001 From: Iva Avramova Date: Sun, 14 Apr 2024 12:59:57 +0300 Subject: [PATCH 2/5] Fix of exception handler trying to write response to a disconnected client Why: On Conductor server side, an exception gets raised `Failure in @ExceptionHandler com.netflix.conductor.rest.controllers.ApplicationExceptionMapper#handleAll(HttpServletRequest, Throwable)`. Cause is attempting to write while the client aborted the connection `org.apache.catalina.connector.ClientAbortException: java.io.IOException: Broken pipe`, while the exception was nested in `ClosedChannelException`. What: Added a ClosedChannelException handler for exception chain containing ClientAbortException. Testing done: Local run. Not trivial to emulate the issue in a test. Such a test may rely on timing out the client before server responded. Signed-off-by: Iva Avramova --- .../ApplicationExceptionMapper.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java index 251144c81..9a536cbe6 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java @@ -14,7 +14,9 @@ import java.nio.channels.ClosedChannelException; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Optional; import org.apache.catalina.connector.ClientAbortException; import org.apache.commons.lang3.exception.ExceptionUtils; @@ -59,10 +61,18 @@ public class ApplicationExceptionMapper { @Order(ValidationExceptionMapper.ORDER + 1) public void handleNestedClientAbortedInClosedChannelException( HttpServletRequest request, ClosedChannelException closedChannelException) { - final Throwable rootCause = ExceptionUtils.getRootCause(closedChannelException); - if (rootCause != null - && ClientAbortException.class.getName().equals(rootCause.getClass().getName())) { - handleClientAborted(request, (ClientAbortException) rootCause); + final List exceptionChain = + ExceptionUtils.getThrowableList(closedChannelException); + final Optional clientAbortedException = + exceptionChain.stream() + .filter( + t -> + ClientAbortException.class + .getName() + .equals(t.getClass().getName())) + .findAny(); + if (clientAbortedException.isPresent()) { + handleClientAborted(request, (ClientAbortException) clientAbortedException.get()); return; } handleAll(request, closedChannelException); From 2e919e535f28a200f8b1494550e7fc023f9a74e6 Mon Sep 17 00:00:00 2001 From: Iva Avramova Date: Mon, 22 Apr 2024 13:58:02 +0300 Subject: [PATCH 3/5] added tests; generalized the exception Signed-off-by: Iva Avramova --- .../ApplicationExceptionMapper.java | 32 ++-- .../ApplicationExceptionMapperTest.java | 151 ++++++++++++++++++ 2 files changed, 163 insertions(+), 20 deletions(-) create mode 100644 rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java index 9a536cbe6..a1d386fa8 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java @@ -12,7 +12,6 @@ */ package com.netflix.conductor.rest.controllers; -import java.nio.channels.ClosedChannelException; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -57,12 +56,18 @@ public class ApplicationExceptionMapper { EXCEPTION_STATUS_MAP.put(NoResourceFoundException.class, HttpStatus.NOT_FOUND); } - @ExceptionHandler(ClosedChannelException.class) + @ExceptionHandler(ClientAbortException.class) @Order(ValidationExceptionMapper.ORDER + 1) - public void handleNestedClientAbortedInClosedChannelException( - HttpServletRequest request, ClosedChannelException closedChannelException) { - final List exceptionChain = - ExceptionUtils.getThrowableList(closedChannelException); + public void handleClientAborted( + HttpServletRequest request, ClientAbortException clientAbortException) { + logException( + request, clientAbortException); // socket closed, cannot return any error response + } + + @ExceptionHandler(Throwable.class) + @Order(ValidationExceptionMapper.ORDER + 2) + public ResponseEntity handleAll(HttpServletRequest request, Throwable th) { + final List exceptionChain = ExceptionUtils.getThrowableList(th); final Optional clientAbortedException = exceptionChain.stream() .filter( @@ -73,22 +78,9 @@ public void handleNestedClientAbortedInClosedChannelException( .findAny(); if (clientAbortedException.isPresent()) { handleClientAborted(request, (ClientAbortException) clientAbortedException.get()); - return; + return ResponseEntity.ok().build(); } - handleAll(request, closedChannelException); - } - @ExceptionHandler(ClientAbortException.class) - @Order(ValidationExceptionMapper.ORDER + 2) - public void handleClientAborted( - HttpServletRequest request, ClientAbortException clientAbortException) { - logException( - request, clientAbortException); // socket closed, cannot return any error response - } - - @ExceptionHandler(Throwable.class) - @Order(ValidationExceptionMapper.ORDER + 3) - public ResponseEntity handleAll(HttpServletRequest request, Throwable th) { logException(request, th); HttpStatus status = diff --git a/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java new file mode 100644 index 000000000..cad107958 --- /dev/null +++ b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java @@ -0,0 +1,151 @@ +/* + * Copyright 2024 Conductor Authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + */ +package com.netflix.conductor.rest.controllers; + +import java.util.Collections; + +import org.apache.catalina.connector.ClientAbortException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.http.MediaType; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; + +import com.netflix.conductor.model.TaskModel; + +import com.fasterxml.jackson.databind.ObjectMapper; + +import static org.mockito.Mockito.*; +import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +public class ApplicationExceptionMapperTest { + + private QueueAdminResource queueAdminResource; + + private MockMvc mockMvc; + + private static MockedStatic mockLoggerFactory; + private static final Logger logger = mock(Logger.class); + + @Before + public void before() { + mockLoggerFactory = Mockito.mockStatic(LoggerFactory.class); + when(LoggerFactory.getLogger(ApplicationExceptionMapper.class)).thenReturn(logger); + + this.queueAdminResource = mock(QueueAdminResource.class); + this.mockMvc = + MockMvcBuilders.standaloneSetup(this.queueAdminResource) + .setControllerAdvice(new ApplicationExceptionMapper()) + .build(); + } + + @After + public void after() { + mockLoggerFactory.close(); + } + + @Test + public void testException() throws Exception { + var exception = new Exception(); + // pick a method that raises a generic exception + doThrow(exception).when(this.queueAdminResource).update(any(), any(), any(), any()); + + // verify we do send an error response + this.mockMvc + .perform( + MockMvcRequestBuilders.post( + "/api/queue/update/workflowId/taskRefName/{status}", + TaskModel.Status.SKIPPED) + .contentType(MediaType.APPLICATION_JSON) + .content( + new ObjectMapper() + .writeValueAsString(Collections.emptyMap()))) + .andDo(print()) + .andExpect(status().is5xxServerError()); + // verify the error was logged + verify(logger) + .error( + "Error {} url: '{}'", + "Exception", + "/api/queue/update/workflowId/taskRefName/SKIPPED", + exception); + verifyNoMoreInteractions(logger); + } + + @Test + public void testClientAbortException() throws Exception { + var clientAbortException = new ClientAbortException("test"); + // pick a method that raises a generic exception + doThrow(clientAbortException) + .when(this.queueAdminResource) + .update(any(), any(), any(), any()); + + // verify we do not try to write response to an already disconnected client + this.mockMvc + .perform( + MockMvcRequestBuilders.post( + "/api/queue/update/workflowId/taskRefName/{status}", + TaskModel.Status.SKIPPED) + .contentType(MediaType.APPLICATION_JSON) + .content( + new ObjectMapper() + .writeValueAsString(Collections.emptyMap()))) + .andDo(print()) + .andExpect(status().isOk()); + // verify the error was logged + verify(logger) + .error( + "Error {} url: '{}'", + "ClientAbortException", + "/api/queue/update/workflowId/taskRefName/SKIPPED", + clientAbortException); + verifyNoMoreInteractions(logger); + } + + @Test + public void testClientAbortExceptionChained() throws Exception { + var clientAbortException = new ClientAbortException("test", new Exception("nested")); + // pick a method that raises a generic exception + doThrow(new RuntimeException(clientAbortException)) + .when(this.queueAdminResource) + .update(any(), any(), any(), any()); + + // verify we do not try to write response to an already disconnected client + this.mockMvc + .perform( + MockMvcRequestBuilders.post( + "/api/queue/update/workflowId/taskRefName/{status}", + TaskModel.Status.SKIPPED) + .contentType(MediaType.APPLICATION_JSON) + .content( + new ObjectMapper() + .writeValueAsString(Collections.emptyMap()))) + .andDo(print()) + .andExpect(status().isOk()); + // verify the error was logged + verify(logger) + .error( + "Error {} url: '{}'", + "ClientAbortException", + "/api/queue/update/workflowId/taskRefName/SKIPPED", + clientAbortException); + verifyNoMoreInteractions(logger); + } +} From ef267ad3863e184b337573793bb91da8db9f6d87 Mon Sep 17 00:00:00 2001 From: Iva Avramova Date: Mon, 22 Apr 2024 14:12:12 +0300 Subject: [PATCH 4/5] added tests; generalized the exception Signed-off-by: Iva Avramova --- .../rest/controllers/ApplicationExceptionMapper.java | 4 +++- .../rest/controllers/ApplicationExceptionMapperTest.java | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java index a1d386fa8..686ba5017 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java @@ -25,6 +25,7 @@ import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestControllerAdvice; import org.springframework.web.servlet.resource.NoResourceFoundException; @@ -58,6 +59,7 @@ public class ApplicationExceptionMapper { @ExceptionHandler(ClientAbortException.class) @Order(ValidationExceptionMapper.ORDER + 1) + @ResponseStatus(HttpStatus.NO_CONTENT) public void handleClientAborted( HttpServletRequest request, ClientAbortException clientAbortException) { logException( @@ -78,7 +80,7 @@ public ResponseEntity handleAll(HttpServletRequest request, Throw .findAny(); if (clientAbortedException.isPresent()) { handleClientAborted(request, (ClientAbortException) clientAbortedException.get()); - return ResponseEntity.ok().build(); + return ResponseEntity.noContent().build(); } logException(request, th); diff --git a/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java index cad107958..52770fd0f 100644 --- a/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java +++ b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java @@ -108,7 +108,7 @@ public void testClientAbortException() throws Exception { new ObjectMapper() .writeValueAsString(Collections.emptyMap()))) .andDo(print()) - .andExpect(status().isOk()); + .andExpect(status().isNoContent()); // verify the error was logged verify(logger) .error( @@ -138,7 +138,7 @@ public void testClientAbortExceptionChained() throws Exception { new ObjectMapper() .writeValueAsString(Collections.emptyMap()))) .andDo(print()) - .andExpect(status().isOk()); + .andExpect(status().isNoContent()); // verify the error was logged verify(logger) .error( From 0376d7cbdc93ee4ead77e2b9e368f84bee7b51ef Mon Sep 17 00:00:00 2001 From: Iva Avramova Date: Mon, 22 Apr 2024 14:38:02 +0300 Subject: [PATCH 5/5] Why: Ignoring a particular exception based on its stack trace should be rather an alerting concern. What: Reverted ClientAbortedException specific handler. Added test coverage for the existing exception handler. Testing done: local run Signed-off-by: Iva Avramova --- .../ApplicationExceptionMapper.java | 30 +-------- .../ApplicationExceptionMapperTest.java | 61 ------------------- 2 files changed, 1 insertion(+), 90 deletions(-) diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java index 686ba5017..486853b9f 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapper.java @@ -13,19 +13,14 @@ package com.netflix.conductor.rest.controllers; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; -import org.apache.catalina.connector.ClientAbortException; -import org.apache.commons.lang3.exception.ExceptionUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.core.annotation.Order; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.ExceptionHandler; -import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestControllerAdvice; import org.springframework.web.servlet.resource.NoResourceFoundException; @@ -40,6 +35,7 @@ import jakarta.servlet.http.HttpServletRequest; @RestControllerAdvice +@Order(ValidationExceptionMapper.ORDER + 1) public class ApplicationExceptionMapper { private static final Logger LOGGER = LoggerFactory.getLogger(ApplicationExceptionMapper.class); @@ -57,32 +53,8 @@ public class ApplicationExceptionMapper { EXCEPTION_STATUS_MAP.put(NoResourceFoundException.class, HttpStatus.NOT_FOUND); } - @ExceptionHandler(ClientAbortException.class) - @Order(ValidationExceptionMapper.ORDER + 1) - @ResponseStatus(HttpStatus.NO_CONTENT) - public void handleClientAborted( - HttpServletRequest request, ClientAbortException clientAbortException) { - logException( - request, clientAbortException); // socket closed, cannot return any error response - } - @ExceptionHandler(Throwable.class) - @Order(ValidationExceptionMapper.ORDER + 2) public ResponseEntity handleAll(HttpServletRequest request, Throwable th) { - final List exceptionChain = ExceptionUtils.getThrowableList(th); - final Optional clientAbortedException = - exceptionChain.stream() - .filter( - t -> - ClientAbortException.class - .getName() - .equals(t.getClass().getName())) - .findAny(); - if (clientAbortedException.isPresent()) { - handleClientAborted(request, (ClientAbortException) clientAbortedException.get()); - return ResponseEntity.noContent().build(); - } - logException(request, th); HttpStatus status = diff --git a/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java index 52770fd0f..c800d5b3f 100644 --- a/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java +++ b/rest/src/test/java/com/netflix/conductor/rest/controllers/ApplicationExceptionMapperTest.java @@ -14,7 +14,6 @@ import java.util.Collections; -import org.apache.catalina.connector.ClientAbortException; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -88,64 +87,4 @@ public void testException() throws Exception { exception); verifyNoMoreInteractions(logger); } - - @Test - public void testClientAbortException() throws Exception { - var clientAbortException = new ClientAbortException("test"); - // pick a method that raises a generic exception - doThrow(clientAbortException) - .when(this.queueAdminResource) - .update(any(), any(), any(), any()); - - // verify we do not try to write response to an already disconnected client - this.mockMvc - .perform( - MockMvcRequestBuilders.post( - "/api/queue/update/workflowId/taskRefName/{status}", - TaskModel.Status.SKIPPED) - .contentType(MediaType.APPLICATION_JSON) - .content( - new ObjectMapper() - .writeValueAsString(Collections.emptyMap()))) - .andDo(print()) - .andExpect(status().isNoContent()); - // verify the error was logged - verify(logger) - .error( - "Error {} url: '{}'", - "ClientAbortException", - "/api/queue/update/workflowId/taskRefName/SKIPPED", - clientAbortException); - verifyNoMoreInteractions(logger); - } - - @Test - public void testClientAbortExceptionChained() throws Exception { - var clientAbortException = new ClientAbortException("test", new Exception("nested")); - // pick a method that raises a generic exception - doThrow(new RuntimeException(clientAbortException)) - .when(this.queueAdminResource) - .update(any(), any(), any(), any()); - - // verify we do not try to write response to an already disconnected client - this.mockMvc - .perform( - MockMvcRequestBuilders.post( - "/api/queue/update/workflowId/taskRefName/{status}", - TaskModel.Status.SKIPPED) - .contentType(MediaType.APPLICATION_JSON) - .content( - new ObjectMapper() - .writeValueAsString(Collections.emptyMap()))) - .andDo(print()) - .andExpect(status().isNoContent()); - // verify the error was logged - verify(logger) - .error( - "Error {} url: '{}'", - "ClientAbortException", - "/api/queue/update/workflowId/taskRefName/SKIPPED", - clientAbortException); - verifyNoMoreInteractions(logger); - } }