Open lbeschastny opened 2 months ago
Hello @lbeschastny, I would to work on this with you but it seem like it is sait that "The intent is that when a circuit times out, an assumption is made that all other ongoing requests to the same endpoint will time out as well". That is why the subsequent request failed. I think what we need is a reset timeout for the abort controller.
@cedrick-ah the problem here is not about other ongoing requests since this limitation is clearly stated in the readme.
The problem is that all new requests are immediately aborted by fetch
after the first timeout due to the AbortController
state which is changed to aborted
the first time the abort signal is sent.
Okay, maybe you can try this approach to change abort for each request.
const { describe, it, before, after } = require('node:test');
const assert = require('node:assert/strict');
const http = require('node:http');
const CircuitBreaker = require('opossum');
describe('Circuit breaker with abort controller', () => {
let abortController;
const get = timeout => {
const { port } = server.address();
abortController = new AbortController();
const { signal } = abortController;
const headers = { 'x-timeout': timeout };
return fetch(`http://localhost:${port}`, { signal, headers });
};
const breaker = new CircuitBreaker(get, {
abortController,
timeout: 1000,
volumeThreshold: 3
});
const server = http.createServer((req, res) => {
const timeout = Number(req.headers['x-timeout']);
setTimeout(() => res.end(`delayed for ${timeout}ms`), timeout);
});
before(() => server.listen());
after(() => server.close());
it('Should work', async () => {
const response = await breaker.fire(100);
const text = await response.text();
assert.equal(text, 'delayed for 100ms');
});
it('Should timeout', async () => {
await assert.rejects(() => breaker.fire(2000), {
message: 'Timed out after 1000ms'
});
});
it('Should work again', async () => {
try {
await breaker.fire(100);
} catch ({ name, message }) {
throw Object.assign(new Error(message), { name });
}
});
});
node --test
▶ Circuit breaker with abort controller
✔ Should work (286.944232ms)
✔ Should timeout (1006.156862ms)
✔ Should work again (117.404018ms)
▶ Circuit breaker with abort controller (1430.090547ms)
ℹ tests 3
ℹ suites 1
ℹ pass 3
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 7338.956067
@cedrick-ah you create new AbortController
for every request here, but for opossum
the abortController
option in your example is undefined
.
So, requests just newer get aborted.
It will be defined after breaker.fire() is called. Also, abort controller and signal only come in pairs as you said earlier. I tested and the new requests was not aborted.
@cedrick-ah I updated my Minimal reproducible example with an additional assertion to verify that the operation was actually aborted.
Okay, I understand.
This issue is stale because it has been open 30 days with no activity.
Node.js Version:
Operating System:
The problem:
AbortSignal
both stores it's state and provides anabort
event to signal when abort is happening.And
AbortController
newer updates it'ssignal
, leaving it inaborted
state forever:It looks like
undici
fetch uses bothaborted
property andabort
event to skip sending the request if it is already aborted.So, once the action times out for the first time, it switches
AbortController
intoaborted
state and all subsequent fetch calls just abort immediately.Minimal reproducible example:
Possible ways to fix this:
It seems we cannot re-use the same
AbortController
andAbortSignal
pair for all requests sinceAbortSignal
preserves itsaborted
state.I can only see one way to fix this problem - use new
AbortSignal
for every requests (see #780).For example,
opossum
could pass thesignal
object as the first argument to the action handler.