Skip to content

Commit

Permalink
[CHANGE] [JS] [CONSUMERS] changed the behaviour of fetch() and next()…
Browse files Browse the repository at this point in the history
… to react to consumer deleted / not found server responses. Since these operations have a termination condition, such events are appropriate. Consume however will keep reporting if there's an error related to the stream or consumer. Clients that want to stop processing during a consume on consumer deleted/stream not found type errors should setup a listener and then call stop on the consumer if those conditions are reported. Order consumers work similarly in next() and fetch() as described above with the exception that consumer not found, really will attempt to recover (as ordered consumers recreate the consumer when there's an error). Applications that use next()/fetch() should trap errors, and re-attempt as desired.
  • Loading branch information
aricart committed Mar 21, 2024
1 parent 5573625 commit ce00862
Show file tree
Hide file tree
Showing 5 changed files with 366 additions and 90 deletions.
78 changes: 51 additions & 27 deletions jetstream/consumer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2022-2023 The NATS Authors
* Copyright 2022-2024 The NATS 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
Expand Down Expand Up @@ -263,11 +263,10 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
pending: { msgs: number; bytes: number; requests: number };
inbox: string;
refilling: boolean;
stack: string;
pong!: Promise<void> | null;
callback: ConsumerCallbackFn | null;
timeout: Timeout<unknown> | null;
cleanupHandler?: () => void;
cleanupHandler?: (err: void | Error) => void;
listeners: QueuedIterator<ConsumerStatus>[];
statusIterator?: QueuedIteratorImpl<Status>;
forOrderedConsumer: boolean;
Expand All @@ -289,7 +288,6 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
this.pong = null;
this.pending = { msgs: 0, bytes: 0, requests: 0 };
this.refilling = refilling;
this.stack = new Error().stack!.split("\n").slice(1).join("\n");
this.timeout = null;
this.inbox = createInbox(c.api.nc.options.inboxPrefix);
this.listeners = [];
Expand All @@ -312,10 +310,10 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
// close is called, the pull consumer will emit a close
// which will close the ordered consumer, by registering
// the close with a handler, we can replace it.
this.closed().then(() => {
this.closed().then((err) => {
if (this.cleanupHandler) {
try {
this.cleanupHandler();
this.cleanupHandler(err);
} catch (_err) {
// nothing
}
Expand All @@ -334,7 +332,7 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
// that the server rejected (eliminating the sub)
// or the client never had permissions to begin with
// so this is terminal
this.stop();
this.stop(err);
return;
}
this.monitor?.work();
Expand Down Expand Up @@ -362,24 +360,20 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
// FIXME: 400 bad request Invalid Heartbeat or Unmarshalling Fails
// these are real bad values - so this is bad request
// fail on this
const toErr = (): Error => {
const err = new NatsError(description, `${code}`);
err.stack += `\n\n${this.stack}`;
return err;
};

// we got a bad request - no progress here
if (code === 400) {
const error = toErr();
//@ts-ignore: fn
this._push(() => {
this.stop(error);
});
this.stop(new NatsError(description, `${code}`));
return;
} else if (code === 409 && description === "consumer deleted") {
this.notify(
ConsumerEvents.ConsumerDeleted,
`${code} ${description}`,
);
if (!this.refilling) {
const error = new NatsError(description, `${code}`);
this.stop(error);
return;
}
} else {
this.notify(
ConsumerDebugEvents.DebugEvent,
Expand Down Expand Up @@ -546,6 +540,10 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
if (err.message === "stream not found") {
streamNotFound++;
this.notify(ConsumerEvents.StreamNotFound, streamNotFound);
if (!this.refilling) {
this.stop(err);
return false;
}
} else if (err.message === "consumer not found") {
notFound++;
this.notify(ConsumerEvents.ConsumerNotFound, notFound);
Expand All @@ -556,6 +554,10 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
// ignored
}
}
if (!this.refilling) {
this.stop(err);
return false;
}
if (this.forOrderedConsumer) {
return false;
}
Expand Down Expand Up @@ -637,7 +639,7 @@ export class PullConsumerMessagesImpl extends QueuedIteratorImpl<JsMsg>
this.timeout = null;
}

setCleanupHandler(fn?: () => void) {
setCleanupHandler(fn?: (err?: void | Error) => void) {
this.cleanupHandler = fn;
}

Expand Down Expand Up @@ -730,8 +732,9 @@ export class OrderedConsumerMessages extends QueuedIteratorImpl<JsMsg>
this.src.stop();
}
this.src = src;
this.src.setCleanupHandler(() => {
this.close().catch();
this.src.setCleanupHandler((err) => {
console.log("cleanup handler");
this.stop(err || undefined);
});
(async () => {
const status = await this.src.status();
Expand All @@ -754,6 +757,9 @@ export class OrderedConsumerMessages extends QueuedIteratorImpl<JsMsg>
}

stop(err?: Error): void {
if (this.done) {
return;
}
this.src?.stop(err);
super.stop(err);
this.listeners.forEach((n) => {
Expand Down Expand Up @@ -809,7 +815,8 @@ export class PullConsumerImpl implements Consumer {
// FIXME: need some way to pad this correctly
const to = Math.round(m.opts.expires * 1.05);
const timer = timeout(to);
m.closed().then(() => {
m.closed().catch(() => {
}).finally(() => {
timer.cancel();
});
timer.catch(() => {
Expand Down Expand Up @@ -852,13 +859,17 @@ export class PullConsumerImpl implements Consumer {
d.resolve(m);
break;
}
})().catch();
})().catch(() => {
// iterator is going to throw, but we ignore it
// as it is handled by the closed promise
});
const timer = timeout(to);
iter.closed().then(() => {
d.resolve(null);
timer.cancel();
iter.closed().then((err) => {
err ? d.reject(err) : d.resolve(null);
}).catch((err) => {
d.reject(err);
}).finally(() => {
timer.cancel();
});
timer.catch((_err) => {
d.resolve(null);
Expand Down Expand Up @@ -1016,6 +1027,16 @@ export class OrderedPullConsumerImpl implements Consumer {
this.iter?.notify(ConsumerEvents.OrderedConsumerRecreated, ci.name);
break;
} catch (err) {
if (err.message === "stream not found") {
// we are not going to succeed
this.iter?.notify(ConsumerEvents.StreamNotFound, i);
// if we are not consume - fail it
if (this.type === PullConsumerType.Fetch) {
this.iter?.stop(err);
return Promise.reject(err);
}
}

if (seq === 0 && i >= 30) {
// consumer was never created, so we can fail this
throw err;
Expand Down Expand Up @@ -1144,7 +1165,10 @@ export class OrderedPullConsumerImpl implements Consumer {
copts as FetchMessages,
) as OrderedConsumerMessages;
iter.iterClosed
.then(() => {
.then((err) => {
if (err) {
d.reject(err);
}
d.resolve(null);
})
.catch((err) => {
Expand Down
137 changes: 102 additions & 35 deletions jetstream/tests/consumers_fetch_test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2022-2023 The NATS Authors
* Copyright 2022-2024 The NATS 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
Expand All @@ -23,7 +23,7 @@ import { AckPolicy, DeliverPolicy } from "../jsapi_types.ts";
import { assertEquals } from "https://deno.land/[email protected]/assert/assert_equals.ts";
import { Empty } from "../../nats-base-client/encoders.ts";
import { StringCodec } from "../../nats-base-client/codec.ts";
import { deferred } from "../../nats-base-client/util.ts";
import { delay } from "../../nats-base-client/util.ts";
import { assertRejects } from "https://deno.land/[email protected]/assert/assert_rejects.ts";
import { nanos } from "../jsutil.ts";
import { NatsConnectionImpl } from "../../nats-base-client/nats.ts";
Expand Down Expand Up @@ -113,39 +113,106 @@ Deno.test("consumers - fetch exactly messages", async () => {
await cleanup(ns, nc);
});

// Deno.test("consumers - fetch deleted consumer", async () => {
// const { ns, nc } = await setup(jetstreamServerConf({}));
// const { stream } = await initStream(nc);
// const jsm = await nc.jetstreamManager();
// await jsm.consumers.add(stream, {
// durable_name: "a",
// ack_policy: AckPolicy.Explicit,
// });
//
// const js = nc.jetstream();
// const c = await js.consumers.get(stream, "a");
// const iter = await c.fetch({
// expires: 30000,
// });
// const dr = deferred();
// setTimeout(() => {
// jsm.consumers.delete(stream, "a")
// .then(() => {
// dr.resolve();
// });
// }, 1000);
// await assertRejects(
// async () => {
// for await (const _m of iter) {
// // nothing
// }
// },
// Error,
// "consumer deleted",
// );
// await dr;
// await cleanup(ns, nc);
// });
Deno.test("consumers - fetch consumer not found", async () => {
const { ns, nc } = await setup(jetstreamServerConf());
const jsm = await nc.jetstreamManager();
await jsm.streams.add({ name: "A", subjects: ["hello"] });

await jsm.consumers.add("A", {
durable_name: "a",
deliver_policy: DeliverPolicy.All,
ack_policy: AckPolicy.Explicit,
});

const js = nc.jetstream();
const c = await js.consumers.get("A", "a");

await c.delete();

const iter = await c.fetch({
expires: 3000,
});

const exited = assertRejects(
async () => {
for await (const _ of iter) {
// nothing
}
},
Error,
"consumer not found",
);

await exited;
await cleanup(ns, nc);
});

Deno.test("consumers - fetch deleted consumer", async () => {
const { ns, nc } = await setup(jetstreamServerConf());
const jsm = await nc.jetstreamManager();
await jsm.streams.add({ name: "A", subjects: ["a"] });

await jsm.consumers.add("A", {
durable_name: "a",
deliver_policy: DeliverPolicy.All,
ack_policy: AckPolicy.Explicit,
});

const js = nc.jetstream();
const c = await js.consumers.get("A", "a");

const iter = await c.fetch({
expires: 3000,
});

const exited = assertRejects(
async () => {
for await (const _ of iter) {
// nothing
}
},
Error,
"consumer deleted",
);

await delay(1000);
await c.delete();

await exited;
await cleanup(ns, nc);
});

Deno.test("consumers - fetch stream not found", async () => {
const { ns, nc } = await setup(jetstreamServerConf());

const jsm = await nc.jetstreamManager();
await jsm.streams.add({ name: "A", subjects: ["hello"] });

await jsm.consumers.add("A", {
durable_name: "a",
deliver_policy: DeliverPolicy.All,
ack_policy: AckPolicy.Explicit,
});

const js = nc.jetstream();
const c = await js.consumers.get("A", "a");
const iter = await c.fetch({
expires: 3000,
});
await jsm.streams.delete("A");

await assertRejects(
async () => {
for await (const _ of iter) {
// nothing
}
},
Error,
"stream not found",
);

await cleanup(ns, nc);
});

Deno.test("consumers - fetch listener leaks", async () => {
const { ns, nc } = await setup(jetstreamServerConf());
Expand Down
Loading

0 comments on commit ce00862

Please sign in to comment.