Skip to content

Commit ec0474c

Browse files
arturobernalgarturobernalg
authored
Fix SSE reconnect handling and scheduler ownership (#800)
1 parent 784177e commit ec0474c

File tree

2 files changed

+274
-2
lines changed

2 files changed

+274
-2
lines changed

‎httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/DefaultEventSource.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,11 +183,10 @@ public DefaultEventSource(final CloseableHttpAsyncClient client,
183183

184184
if (scheduler != null) {
185185
this.scheduler = scheduler;
186-
this.ownScheduler = scheduler != SHARED_SCHED;
187186
} else {
188187
this.scheduler = SHARED_SCHED;
189-
this.ownScheduler = false;
190188
}
189+
this.ownScheduler = false;
191190

192191
this.callbackExecutor = callbackExecutor != null ? callbackExecutor : Runnable::run;
193192

@@ -352,6 +351,11 @@ public void completed(final Void v) {
352351
@Override
353352
public void failed(final Exception ex) {
354353
connected.set(false);
354+
if (ex instanceof SseResponseConsumer.StopReconnectException) {
355+
dispatch(() -> listener.onFailure(ex, false));
356+
notifyClosedOnce();
357+
return;
358+
}
355359
if (cancelled.get() || isBenignCancel(ex)) {
356360
notifyClosedOnce();
357361
return;

‎httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/impl/DefaultEventSourceTest.java‎

Lines changed: 268 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,268 @@
1+
/*
2+
* ====================================================================
3+
* Licensed to the Apache Software Foundation (ASF) under one
4+
* or more contributor license agreements. See the NOTICE file
5+
* distributed with this work for additional information
6+
* regarding copyright ownership. The ASF licenses this file
7+
* to you under the Apache License, Version 2.0 (the
8+
* "License"); you may not use this file except in compliance
9+
* with the License. You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing,
14+
* software distributed under the License is distributed on an
15+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
16+
* KIND, either express or implied. See the License for the
17+
* specific language governing permissions and limitations
18+
* under the License.
19+
* ====================================================================
20+
*
21+
* This software consists of voluntary contributions made by many
22+
* individuals on behalf of the Apache Software Foundation. For more
23+
* information on the Apache Software Foundation, please see
24+
* <http://www.apache.org/>.
25+
*
26+
*/
27+
package org.apache.hc.client5.http.sse.impl;
28+
29+
import static org.junit.jupiter.api.Assertions.assertFalse;
30+
import static org.junit.jupiter.api.Assertions.assertTrue;
31+
32+
import java.net.URI;
33+
import java.util.Collections;
34+
import java.util.List;
35+
import java.util.concurrent.AbstractExecutorService;
36+
import java.util.concurrent.Callable;
37+
import java.util.concurrent.CompletableFuture;
38+
import java.util.concurrent.CountDownLatch;
39+
import java.util.concurrent.Delayed;
40+
import java.util.concurrent.Future;
41+
import java.util.concurrent.ScheduledExecutorService;
42+
import java.util.concurrent.ScheduledFuture;
43+
import java.util.concurrent.TimeUnit;
44+
import java.util.concurrent.atomic.AtomicBoolean;
45+
import java.util.concurrent.atomic.AtomicInteger;
46+
47+
import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient;
48+
import org.apache.hc.client5.http.sse.EventSourceListener;
49+
import org.apache.hc.core5.concurrent.FutureCallback;
50+
import org.apache.hc.core5.function.Supplier;
51+
import org.apache.hc.core5.http.HttpHost;
52+
import org.apache.hc.core5.http.nio.AsyncPushConsumer;
53+
import org.apache.hc.core5.http.nio.AsyncRequestProducer;
54+
import org.apache.hc.core5.http.nio.AsyncResponseConsumer;
55+
import org.apache.hc.core5.http.nio.HandlerFactory;
56+
import org.apache.hc.core5.http.protocol.HttpContext;
57+
import org.apache.hc.core5.io.CloseMode;
58+
import org.apache.hc.core5.reactor.IOReactorStatus;
59+
import org.apache.hc.core5.util.TimeValue;
60+
import org.junit.jupiter.api.Test;
61+
62+
class DefaultEventSourceTest {
63+
64+
@Test
65+
void stopReconnectExceptionDoesNotScheduleReconnect() throws Exception {
66+
final RecordingScheduler scheduler = new RecordingScheduler();
67+
final CapturingClient client = new CapturingClient();
68+
final CountDownLatch failure = new CountDownLatch(1);
69+
final CountDownLatch closed = new CountDownLatch(1);
70+
71+
final EventSourceListener listener = new EventSourceListener() {
72+
@Override
73+
public void onFailure(final Throwable t, final boolean willReconnect) {
74+
if (!willReconnect) {
75+
failure.countDown();
76+
}
77+
}
78+
79+
@Override
80+
public void onClosed() {
81+
closed.countDown();
82+
}
83+
84+
@Override
85+
public void onEvent(final String id, final String type, final String data) {
86+
}
87+
};
88+
89+
final DefaultEventSource es = new DefaultEventSource(
90+
client,
91+
URI.create("http://example.org/sse"),
92+
Collections.emptyMap(),
93+
listener,
94+
scheduler,
95+
null,
96+
null,
97+
SseParser.CHAR);
98+
99+
es.start();
100+
client.lastCallback.failed(new SseResponseConsumer.StopReconnectException("Server closed stream (204)"));
101+
102+
assertTrue(failure.await(1, TimeUnit.SECONDS), "failure observed");
103+
assertTrue(closed.await(1, TimeUnit.SECONDS), "closed observed");
104+
assertTrue(scheduler.scheduledCount.get() == 0, "no reconnect scheduled");
105+
}
106+
107+
@Test
108+
void callerSchedulerIsNotShutdownOnCancel() {
109+
final RecordingScheduler scheduler = new RecordingScheduler();
110+
final CapturingClient client = new CapturingClient();
111+
112+
final DefaultEventSource es = new DefaultEventSource(
113+
client,
114+
URI.create("http://example.org/sse"),
115+
Collections.emptyMap(),
116+
(id, type, data) -> { },
117+
scheduler,
118+
null,
119+
null,
120+
SseParser.CHAR);
121+
122+
es.start();
123+
es.cancel();
124+
125+
assertFalse(scheduler.shutdownCalled.get(), "caller scheduler not shutdown");
126+
}
127+
128+
static final class CapturingClient extends CloseableHttpAsyncClient {
129+
volatile FutureCallback<Void> lastCallback;
130+
131+
@Override
132+
public void start() { }
133+
134+
@Override
135+
public IOReactorStatus getStatus() {
136+
return IOReactorStatus.ACTIVE;
137+
}
138+
139+
@Override
140+
public void awaitShutdown(final TimeValue waitTime) throws InterruptedException { }
141+
142+
@Override
143+
public void initiateShutdown() { }
144+
145+
@Override
146+
public void close(final CloseMode closeMode) { }
147+
148+
@Override
149+
public void close() { }
150+
151+
@Override
152+
protected <T> Future<T> doExecute(
153+
final HttpHost target,
154+
final AsyncRequestProducer requestProducer,
155+
final AsyncResponseConsumer<T> responseConsumer,
156+
final HandlerFactory<AsyncPushConsumer> pushHandlerFactory,
157+
final HttpContext context,
158+
final FutureCallback<T> callback) {
159+
@SuppressWarnings("unchecked") final FutureCallback<Void> cb = (FutureCallback<Void>) callback;
160+
this.lastCallback = cb;
161+
return new CompletableFuture<>();
162+
}
163+
164+
@Override
165+
@Deprecated
166+
public void register(final String hostname, final String uriPattern, final Supplier<AsyncPushConsumer> supplier) {
167+
}
168+
}
169+
170+
static final class RecordingScheduler extends AbstractExecutorService implements ScheduledExecutorService {
171+
final AtomicBoolean shutdownCalled = new AtomicBoolean(false);
172+
final AtomicBoolean shutdown = new AtomicBoolean(false);
173+
final AtomicInteger scheduledCount = new AtomicInteger(0);
174+
175+
@Override
176+
public void shutdown() {
177+
shutdown.set(true);
178+
}
179+
180+
@Override
181+
public List<Runnable> shutdownNow() {
182+
shutdownCalled.set(true);
183+
shutdown.set(true);
184+
return Collections.emptyList();
185+
}
186+
187+
@Override
188+
public boolean isShutdown() {
189+
return shutdown.get();
190+
}
191+
192+
@Override
193+
public boolean isTerminated() {
194+
return shutdown.get();
195+
}
196+
197+
@Override
198+
public boolean awaitTermination(final long timeout, final TimeUnit unit) {
199+
return true;
200+
}
201+
202+
@Override
203+
public void execute(final Runnable command) {
204+
command.run();
205+
}
206+
207+
@Override
208+
public ScheduledFuture<?> schedule(final Runnable command, final long delay, final TimeUnit unit) {
209+
scheduledCount.incrementAndGet();
210+
return new DummyScheduledFuture<>();
211+
}
212+
213+
@Override
214+
public <V> ScheduledFuture<V> schedule(final Callable<V> callable, final long delay, final TimeUnit unit) {
215+
scheduledCount.incrementAndGet();
216+
return new DummyScheduledFuture<>();
217+
}
218+
219+
@Override
220+
public ScheduledFuture<?> scheduleAtFixedRate(
221+
final Runnable command, final long initialDelay, final long period, final TimeUnit unit) {
222+
throw new UnsupportedOperationException("not used");
223+
}
224+
225+
@Override
226+
public ScheduledFuture<?> scheduleWithFixedDelay(
227+
final Runnable command, final long initialDelay, final long delay, final TimeUnit unit) {
228+
throw new UnsupportedOperationException("not used");
229+
}
230+
}
231+
232+
static final class DummyScheduledFuture<V> implements ScheduledFuture<V> {
233+
@Override
234+
public long getDelay(final TimeUnit unit) {
235+
return 0;
236+
}
237+
238+
@Override
239+
public int compareTo(final Delayed o) {
240+
return 0;
241+
}
242+
243+
@Override
244+
public boolean cancel(final boolean mayInterruptIfRunning) {
245+
return false;
246+
}
247+
248+
@Override
249+
public boolean isCancelled() {
250+
return false;
251+
}
252+
253+
@Override
254+
public boolean isDone() {
255+
return false;
256+
}
257+
258+
@Override
259+
public V get() {
260+
return null;
261+
}
262+
263+
@Override
264+
public V get(final long timeout, final TimeUnit unit) {
265+
return null;
266+
}
267+
}
268+
}

0 commit comments

Comments
 (0)