Aias00 commented on code in PR #7160:
URL: https://github.com/apache/shenyu/pull/7160#discussion_r4071762702
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/http/HttpLongPollingDataChangedListener.java:
##########
@@ -80,6 +79,8 @@
@SuppressWarnings("all")
public class HttpLongPollingDataChangedListener extends
AbstractDataChangedListener {
+ private static final int SCHEDULER_THREADS = 4;
Review Comment:
Two concerns with widening the scheduler here.
`LongPollingClient#sendResponse` is not idempotent:
```java
void sendResponse(final List<ConfigGroupEnum> changedGroups) {
if (Objects.nonNull(asyncTimeoutFuture)) {
asyncTimeoutFuture.cancel(false);
}
generateResponse((HttpServletResponse) asyncContext.getResponse(),
changedGroups);
asyncContext.complete();
}
```
A client that watches several groups is registered once per `(namespaceId,
group)` key, so the same `LongPollingClient` instance can sit in more than one
`clientsMap` queue. Previously the scheduler had a single thread, so two
notifications (say PLUGIN and RULE) could never `sendResponse` the same client
concurrently — the tasks were serialised.
With `SCHEDULER_THREADS = 4`, two `doRun` batches can now run in parallel,
and if they both contain that client you get two concurrent `generateResponse`
writes followed by two `asyncContext.complete()` calls. The second `complete()`
throws `IllegalStateException`, and the writes can interleave in the response
body.
Options:
- make `sendResponse` idempotent (an `AtomicBoolean` guard, or a CAS on a
`responded` flag on the client), which is the robust fix and also protects the
timeout path; or
- keep this PR focused on the namespace scoping and move the thread-count
bump to its own change with its own justification.
Also worth noting: `refreshLocks` is keyed by namespace and never evicted.
That is fine in practice (the number of namespaces is small and bounded), but
if namespaces can be created dynamically it grows without limit — a
`ConcurrentHashMap` of a handful of monitors is not a problem today, just not
something that should silently become unbounded later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
