Skip to content

Commit 2f93c05

Browse files
author
Vladimir Kotal
authored
deal with exceptions in asynchronous API tasks (#3855)
fixes #3854
1 parent 1c25812 commit 2f93c05

File tree

9 files changed

+203
-32
lines changed

9 files changed

+203
-32
lines changed

‎opengrok-web/src/main/java/org/opengrok/web/api/ApiTask.java

Lines changed: 99 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,12 @@
2525
import jakarta.ws.rs.core.Response;
2626
import org.opengrok.indexer.logger.LoggerFactory;
2727

28+
import java.util.HashMap;
29+
import java.util.Map;
2830
import java.util.UUID;
31+
import java.util.concurrent.Callable;
32+
import java.util.concurrent.ExecutionException;
33+
import java.util.concurrent.Future;
2934
import java.util.logging.Level;
3035
import java.util.logging.Logger;
3136

@@ -36,7 +41,7 @@ public class ApiTask {
3641

3742
private static final Logger LOGGER = LoggerFactory.getLogger(ApiTask.class);
3843

39-
private final Runnable runnable;
44+
private final Callable<Object> callable;
4045

4146
enum ApiTaskState {
4247
INITIAL,
@@ -50,57 +55,137 @@ enum ApiTaskState {
5055

5156
private final Response.Status responseStatus;
5257

58+
private Future<Object> future;
59+
60+
private final Map<Class<?>, Response.Status> exceptionStatusMap = new HashMap<>();
61+
5362
/**
5463
* @param path request path (for identification)
55-
* @param runnable Runnable object
64+
* @param callable Callable object
5665
*/
57-
public ApiTask(String path, Runnable runnable) {
58-
this(path, runnable, Response.Status.OK);
66+
public ApiTask(String path, Callable<Object> callable) {
67+
this(path, callable, Response.Status.OK);
5968
}
6069

6170
/**
6271
* @param path request path (for identification)
63-
* @param runnable Runnable object
72+
* @param callable Callable object
6473
* @param status request status to return after the runnable is done
6574
*/
66-
public ApiTask(String path, Runnable runnable, Response.Status status) {
67-
this.runnable = runnable;
75+
public ApiTask(String path, Callable<Object> callable, Response.Status status) {
76+
this(path, callable, status, null);
77+
}
78+
79+
/**
80+
* @param path request path (for identification)
81+
* @param callable Callable object
82+
* @param status request status to return after the runnable is done
83+
* @param exceptionStatusMap map of {@link Exception} to {@link Response.Status}
84+
*/
85+
public ApiTask(String path, Callable<Object> callable, Response.Status status,
86+
Map<Class<?>, Response.Status> exceptionStatusMap) {
87+
this.callable = callable;
6888
this.uuid = UUID.randomUUID();
6989
this.responseStatus = status;
7090
this.path = path;
7191
this.state = ApiTaskState.INITIAL;
92+
if (exceptionStatusMap != null) {
93+
this.exceptionStatusMap.putAll(exceptionStatusMap);
94+
}
7295
}
7396

97+
/**
98+
* The UUID is randomly generated in the constructor.
99+
* @return UUID
100+
*/
74101
public UUID getUuid() {
75102
return uuid;
76103
}
77104

78105
/**
79-
* @return response status
106+
* @return response status to be used when the task was successfully completed
80107
*/
81-
public Response.Status getResponseStatus() {
108+
Response.Status getResponseStatus() {
82109
return responseStatus;
83110
}
84111

85112
/**
86113
* Set status as submitted.
87114
*/
88-
public void setSubmitted() {
115+
void setSubmitted() {
89116
state = ApiTaskState.SUBMITTED;
90117
}
91118

119+
/**
120+
* @return whether the API task successfully completed
121+
*/
92122
public boolean isCompleted() {
93123
return state.equals(ApiTaskState.COMPLETED);
94124
}
95125

96-
public void setCompleted() {
126+
void setCompleted() {
97127
state = ApiTaskState.COMPLETED;
98128
}
99129

130+
/**
131+
* @param future Future object used for tracking the progress of the API task
132+
*/
133+
void setFuture(Future<Object> future) {
134+
this.future = future;
135+
}
136+
137+
/**
138+
* @return whether the task is finished (normally or with exception)
139+
*/
140+
public boolean isDone() {
141+
if (future != null) {
142+
return future.isDone();
143+
} else {
144+
return false;
145+
}
146+
}
147+
148+
/**
149+
* Provides simple Exception to status code mapping. The Exception match is exact, i.e. exception class hierarchy
150+
* is not considered.
151+
* @param exception Exception
152+
* @return Response status
153+
*/
154+
private Response.Status mapExceptionToStatus(ExecutionException exception) {
155+
return exceptionStatusMap.getOrDefault(exception.getCause().getClass(), Response.Status.INTERNAL_SERVER_ERROR);
156+
}
157+
158+
/**
159+
* This method assumes that the API task is finished.
160+
* @return response object corresponding to the state of the API task
161+
* @throws IllegalStateException if the API task is not finished
162+
*/
163+
public Response getResponse() {
164+
// Avoid thread being blocked in future.get() below.
165+
if (!isDone()) {
166+
throw new IllegalStateException(String.format("task %s not yet done", this));
167+
}
168+
169+
Object obj;
170+
try {
171+
obj = future.get();
172+
} catch (InterruptedException ex) {
173+
return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
174+
} catch (ExecutionException ex) {
175+
return Response.status(mapExceptionToStatus(ex)).entity(ex.toString()).build();
176+
}
177+
178+
if (obj != null) {
179+
return Response.status(getResponseStatus()).entity(obj.toString()).build();
180+
}
181+
182+
return Response.status(getResponseStatus()).build();
183+
}
184+
100185
/**
101186
* @return Runnable object that contains the work that needs to be completed for this API request
102187
*/
103-
public Runnable getRunnable() {
188+
public Callable<Object> getCallable() {
104189
synchronized (this) {
105190
if (state.equals(ApiTaskState.SUBMITTED)) {
106191
throw new IllegalStateException(String.format("API task %s already submitted", this));
@@ -109,9 +194,10 @@ public Runnable getRunnable() {
109194
return () -> {
110195
LOGGER.log(Level.FINE, "API task {0} started", this);
111196
setSubmitted();
112-
runnable.run();
197+
Object ret = callable.call();
113198
setCompleted();
114199
LOGGER.log(Level.FINE, "API task {0} done", this);
200+
return ret;
115201
};
116202
}
117203
}

‎opengrok-web/src/main/java/org/opengrok/web/api/ApiTaskManager.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public Response submitApiTask(String name, ApiTask apiTask) {
9292
return Response.status(Response.Status.BAD_REQUEST).build();
9393
}
9494

95-
queues.get(queueName).submit(apiTask.getRunnable());
95+
apiTask.setFuture(queues.get(queueName).submit(apiTask.getCallable()));
9696
apiTasks.put(apiTask.getUuid(), apiTask);
9797

9898
return Response.status(Response.Status.ACCEPTED).

‎opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/ConfigurationController.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ public Response set(@Context HttpServletRequest request,
8080
new ApiTask(request.getRequestURI(), () -> {
8181
env.applyConfig(body, reindex, CommandTimeoutType.RESTFUL);
8282
suggesterService.refresh();
83+
return null;
8384
}));
8485
}
8586

@@ -102,6 +103,7 @@ public Response setField(@Context HttpServletRequest request,
102103
// apply the configuration - let the environment reload the configuration if necessary
103104
env.applyConfig(false, CommandTimeoutType.RESTFUL);
104105
suggesterService.refresh();
106+
return null;
105107
}));
106108
}
107109

@@ -110,7 +112,11 @@ public Response setField(@Context HttpServletRequest request,
110112
public Response reloadAuthorization(@Context HttpServletRequest request) {
111113
return ApiTaskManager.getInstance().submitApiTask("authorization",
112114
new ApiTask(request.getRequestURI(),
113-
() -> env.getAuthorizationFramework().reload(), Response.Status.NO_CONTENT));
115+
() -> {
116+
env.getAuthorizationFramework().reload();
117+
return null;
118+
},
119+
Response.Status.NO_CONTENT));
114120
}
115121

116122
private Object getConfigurationValueException(String fieldName) throws WebApplicationException {

‎opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/ProjectsController.java

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,11 @@ public Response deleteProject(@Context HttpServletRequest request, @PathParam("p
176176

177177
return ApiTaskManager.getInstance().submitApiTask(PROJECTS_PATH,
178178
new ApiTask(request.getRequestURI(),
179-
() -> deleteProjectWorkHorse(projectName, project), Response.Status.NO_CONTENT));
179+
() -> {
180+
deleteProjectWorkHorse(projectName, project);
181+
return null;
182+
},
183+
Response.Status.NO_CONTENT));
180184
}
181185

182186
private void deleteProjectWorkHorse(String projectName, Project project) {
@@ -214,7 +218,11 @@ public Response deleteProjectData(@Context HttpServletRequest request,
214218
disableProject(projectName);
215219

216220
return ApiTaskManager.getInstance().submitApiTask(PROJECTS_PATH,
217-
new ApiTask(request.getRequestURI(), () -> deleteProjectDataWorkHorse(projectName)));
221+
new ApiTask(request.getRequestURI(),
222+
() -> {
223+
deleteProjectDataWorkHorse(projectName);
224+
return null;
225+
}));
218226
}
219227

220228
private void deleteProjectDataWorkHorse(String projectName) {
@@ -249,7 +257,11 @@ public Response deleteHistoryCache(@Context HttpServletRequest request,
249257
final String projectName = Laundromat.launderInput(projectNameParam);
250258

251259
return ApiTaskManager.getInstance().submitApiTask(PROJECTS_PATH,
252-
new ApiTask(request.getRequestURI(), () -> deleteHistoryCacheWorkHorse(projectName)));
260+
new ApiTask(request.getRequestURI(),
261+
() -> {
262+
deleteHistoryCacheWorkHorse(projectName);
263+
return null;
264+
}));
253265
}
254266

255267
private void deleteHistoryCacheWorkHorse(String projectName) {

‎opengrok-web/src/main/java/org/opengrok/web/api/v1/controller/StatusController.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,8 @@ public Response getStatus(@PathParam("uuid") String uuid) {
5555
return Response.status(Response.Status.NOT_FOUND).build();
5656
}
5757

58-
if (apiTask.isCompleted()) {
59-
return Response.status(apiTask.getResponseStatus()).build();
58+
if (apiTask.isDone()) {
59+
return apiTask.getResponse();
6060
} else {
6161
return Response.status(Response.Status.ACCEPTED).build();
6262
}
@@ -70,8 +70,8 @@ public Response delete(@PathParam("uuid") String uuid) {
7070
return Response.status(Response.Status.NOT_FOUND).build();
7171
}
7272

73-
if (!apiTask.isCompleted()) {
74-
LOGGER.log(Level.WARNING, "API task {0} not yet completed", apiTask);
73+
if (!apiTask.isDone()) {
74+
LOGGER.log(Level.WARNING, "API task {0} not yet done", apiTask);
7575
return Response.status(Response.Status.BAD_REQUEST).build();
7676
}
7777

‎opengrok-web/src/test/java/org/opengrok/web/api/ApiTaskManagerTest.java

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,11 @@
2626
import jakarta.ws.rs.core.Response;
2727
import org.junit.jupiter.api.Test;
2828

29+
import java.util.Map;
2930
import java.util.concurrent.RejectedExecutionException;
31+
import java.util.concurrent.TimeUnit;
3032

33+
import static org.awaitility.Awaitility.await;
3134
import static org.junit.jupiter.api.Assertions.assertEquals;
3235
import static org.junit.jupiter.api.Assertions.assertNotNull;
3336
import static org.junit.jupiter.api.Assertions.assertNull;
@@ -49,7 +52,8 @@ void testQueueName() {
4952
assertEquals(name.substring(1), ApiTaskManager.getQueueName(name));
5053
}
5154

52-
private void doNothing() {
55+
private Object doNothing() {
56+
return null;
5357
}
5458

5559
@Test
@@ -77,6 +81,54 @@ void testTaskSubmitDelete() {
7781
assertNull(apiTaskManager.getApiTask(uuidString));
7882
}
7983

84+
@Test
85+
void taskSubmitCallableWithException() {
86+
ApiTaskManager apiTaskManager = ApiTaskManager.getInstance();
87+
String name = "exception";
88+
apiTaskManager.addPool(name, 1);
89+
ApiTask apiTask = new ApiTask("foo",
90+
() -> {
91+
throw new Exception("foo");
92+
});
93+
apiTaskManager.submitApiTask(name, apiTask);
94+
await().atMost(3, TimeUnit.SECONDS).until(apiTask::isDone);
95+
assertEquals(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), apiTask.getResponse().getStatus());
96+
}
97+
98+
@Test
99+
void taskSubmitCallableWithExceptionMapping() {
100+
ApiTaskManager apiTaskManager = ApiTaskManager.getInstance();
101+
String name = "exceptionMap";
102+
apiTaskManager.addPool(name, 1);
103+
final String exceptionText = "exception text";
104+
ApiTask apiTask = new ApiTask("foo",
105+
() -> {
106+
throw new IllegalStateException(exceptionText);
107+
},
108+
Response.Status.NO_CONTENT,
109+
Map.of(IllegalStateException.class, Response.Status.NOT_ACCEPTABLE));
110+
apiTaskManager.submitApiTask(name, apiTask);
111+
await().atMost(3, TimeUnit.SECONDS).until(apiTask::isDone);
112+
Response response = apiTask.getResponse();
113+
assertEquals(Response.Status.NOT_ACCEPTABLE.getStatusCode(), response.getStatus());
114+
assertTrue(response.getEntity().toString().contains(exceptionText));
115+
}
116+
117+
@Test
118+
void testCallable() {
119+
ApiTaskManager apiTaskManager = ApiTaskManager.getInstance();
120+
String name = "payload";
121+
apiTaskManager.addPool(name, 1);
122+
final String payloadText = "payload text";
123+
ApiTask apiTask = new ApiTask("payload", () -> payloadText);
124+
apiTaskManager.submitApiTask(name, apiTask);
125+
await().atMost(3, TimeUnit.SECONDS).until(apiTask::isDone);
126+
Response response = apiTask.getResponse();
127+
assertEquals(Response.Status.OK.getStatusCode(), response.getStatus());
128+
assertNotNull(response.getEntity());
129+
assertTrue(response.getEntity().toString().contains(payloadText));
130+
}
131+
80132
@Test
81133
void testTaskInvalidUuid() {
82134
ApiTaskManager apiTaskManager = ApiTaskManager.getInstance();

0 commit comments

Comments
 (0)