Is it correct that consumer.Consume() is the only way to consume a stream with graceful shutdown?

[tpihl]

To follow the pattern of starting something and then defer a something.Close(), is the only option available in the new api; consumer.Consume() with the special returned consumContext that is not a context and exposes a single Stop menthod?

It would be more intuitive to give it a context and cancel it to make it stop. But that would (?) break the use of context to cancel the api-method itself.

But it raises the question, what is it the context actually cancel? In what situation do we expect to cancel a AddConsumer's progress? Or the DeleteConsumer. If i cancel the AddConsumer, what happens if the consumer actually is created but i havent yet received the ack from the server? Will it get removed again? Maybe the error will be context.Cancelled, but does that mean everything the the method may do is not done (like a transactional rollback). Or is there any api-calls that do more than one thing and cancelling them may leave us in an uncertain state?

I absolutly agree using ctx on Fetch and similar things expected to take time where a partial answer might be acceptable. Also in publish since quorum/persisting may take longer than expected. But I cannot really understand it's use in calls like AddStream, PublishAsync, AddConsumer, input parameter in Consume, DeleteConsumer.

Can anyone help me understand better? Am I missunderstanding the concept of context (to stop lengthy operations when there reason to do the operation was removed)?

[piotrpio]

Stream/Consumer management methods accept context because these are synchronous operation which may take some time, so for me it makes sense to supply this timeout to them, wlthough there is no rollback mechanism AFAIK, so if the client does not receive the response but server creates the asset then yes, that leaves us in uncertain state. However, that does not change the fact that you do need a timeout on those operations, and setting API request timeout/cancellation through context makes a lot of sense to me. It's really the same as having context on Publish, for which you noted it makes sense.

As for PublishAsync(), you're absolutely right, having ctx as parameter makes zero sense, I will remove that.

Furthermore, we did not want to add context on Consume() and Messages(), because we do not want the global context used by the users to accidentally cancel the operation with timeout. For those operations, having single Stop() (which can be called as part of your application's graceful shutdown) makes a bit more sense.

[tpihl]

I hope I can dig a (maybe not a) little bit deeper to understand, because this usage of the context.

Reading the reason for the context package (https://go.dev/blog/context) it was always my understanding that the intended use was to reduce wasted resources on (long) running executions where the caller may loose interest before the answer is ready to be sent.

So using context in the server to be able to abort expensive operations if the client disconnects makes all kind of sense.

Reading further it also makes sense if the reason the client tries to get information that may take some time, it can intentionally tell the server that the interest is lost without the actual need to disconnect.

So for any situations where there is a lot of data that is processed incrementally in the server (read or possibly also write), it can/will save resources if we can cancel the processing while it's running. However if we're in the middle of a big write, that is likely an indication of another problem. I cannot find a real use-case where an abort mid-write makes sense. However any read that still have a queue of not yet processed items is if course benefiting from an early cancel, perfect use of context.

As for all the admin calls (creating/updating/deleting streams/consumer), how can they reduce resource waste by a cancellation. They are one or two round trips and most of them seems to block for a very short time in the server. As I understand it, the only server-side situation where it may take significant time is when there is a malfunction or recovery running. And the only other scenario is that the the request or response was lost. Neither case will reasonably be able to reduce wasted resources. Anything that is one or two round trips and normally very fast should not need a context. All the aggressive caching/pre-loading in the client will most likely anyhow negate any benefit a cancel can get in the server for all the Info/Metadata/Streams() and similar.

My feeling is that we make the API significantly more complicated to use for a very insignificant gain. If we could get a "reversed" inProgress from the server when it have spent some short time (or even when it can predict that it cannot answer really fast, maybe because of a huge Fetch or because of performance issues), we could implement our decision in the error callback instead of littered everywhere in the code.

Frankly, using contexts everywhere makes the code harder to read and understand. It's not simple boilerplate, there is now a need to keep track of what contexts is based on what contexts, we create a huge amount of them that needs to be GC'd when they are cancelled/times out and so on.

A simplified client should be simpler to understand and use.

My suggestion;

• Lets try the ConsumerInfoListers Err() channel to remove a lot of per-call-error-checking (that would make it much easier to use the good function-names to chain call and make it read really well). if err:=js.DeleteConsumer(consumer.Info().Name)... it way easier that a [create context with timeout, call consumer.Info((), check it's error, create a new context with timeout, call DeleteConsumer and check it's error]

• Lets use some reasonable timeout default, overridable when calling jetstream.New(nc, opts...) and use any other means to detect failures/reason to return instead of require all the additional boilerplate code if contexts for everything except when we read chunks of data (or write chunks but i cannot see that we do chunk sync writes of many msgs)

• Lets focus on trying to make the interface as easy as possible to understand when reading the code using it.

• Let's re-use the patterns already established in the std library (like bufio.Scanner) since if we already understand that pattern and things work as we intuitively expect (I agree that std lib have some solutions that could be better, but we all learn and live with them, so lets use that knowledge).

• Either use the Closer interface to stop/gracefully terminate something or allow an optional read-channel that the caller can close to trigger a graceful shutdown. Not receiving a Fetsh for a long time is not a sure indication that the interest is lost, not even for an ephemeral consumer

Even wilder, concept is to allow context in the AddConsumer that the caller can cancel when they loose the interest in the long running expensive operation that a consumer is (I agree that this too wasn't what context intended to solve, but it's very nice to be able to call cancel any number of times without panics on closed channels and other racy stuff..

I understand that this may to be long for most people, but it is written with the best intents and only to benefit us all by trying to fully explain thing for one (my) perspective

[piotrpio]

Thanks so much for the feedback!

Starting with the usage of context. We do have to handle timeouts and cancellations in management calls. Server API calls could easily fail or take some time, this is the nature of working over the network - creating a stream or fetching stream info can sometimes take even a couple of seconds. And you're right, cancelling the context will not release any resources on the server - but it will on the client. Having context.Context as a way to handle timeouts and cancellation is really an idiomatic approach in go, as is context management in general - it is more flexible than simply having a timeout on operation.

One thing we may change, hopefully not to your horror - we may want to pass context.Context to Fetch(), as you're right that now it is not possible to gracefully cancel this operation. That way we would be handling both timeout and cancellation in a single place, as we do in other places in jetstream package.

Now let's go through your suggestions:

Lets try the ConsumerInfoListers Err() channel to remove a lot of per-call-error-checking (that would make it much easier to use the good function-names to chain call and make it read really well). if err:=js.DeleteConsumer(consumer.Info().Name)... it way easier that a [create context with timeout, call consumer.Info((), check it's error, create a new context with timeout, call DeleteConsumer and check it's error]

Not sure I really get the first part with ConsumerInfoListers, but I understand you want some of the methods to not return error and expose method to check the error, is that correct? If so, I am not convinced - e.g. chaining Info() really is quite dangerous as you can get nil response, which would obviously panic. However, as to your example usage - we are exposing a CachedInfo() method to get to the most recently fetched info of a stream/consumer - e.g. creating/fetching a consumer returns Stream/Consumer which already has info stored - you can use CachedInfo() to retrieve it, or call Info() to get the up-to-date info. So the DeleteConsumer() example would be working fine if you used

err:=js.DeleteConsumer(consumer.CachedInfo().Name)

Lets use some reasonable timeout default, overridable when calling jetstream.New(nc, opts...) and use any other means to detect failures/reason to return instead of require all the additional boilerplate code if contexts for everything except when we read chunks of data (or write chunks but i cannot see that we do chunk sync writes of many msgs)

Already answered - I think context should be the required first parameter and I would not want more than one way of handling timeouts.

Lets focus on trying to make the interface as easy as possible to understand when reading the code using it.

As you suggested in other discussion, I agree that we should document the interfaces better by at least adding argument names to signatures, I will make that change.

Let's re-use the patterns already established in the std library (like bufio.Scanner) since if we already understand that pattern and things work as we intuitively expect (I agree that std lib have some solutions that could be better, but we all learn and live with them, so lets use that knowledge).

Already answered in other discussion (#1283). Moreover, there really is no idiomatic way of implementing iterators in std lib. For reference, I suggest reading the standard iterator proposal for go: golang/go#54245. For reference:

  Go does have examples of non-generic iterators:
   - runtime.CallersFrames returns a runtime.Frames that iterates over stack frames; Frames has a Next method that returns a Frame and a bool that reports whether there are more frames.
   - bufio.Scanner is an iterator through an io.Reader, where the Scan method advances to the next value. The value is returned by a Bytes method. Errors are collected and returned by an Err method.
   - database/sql.Rows iterates through the results of a query, where the Next method advances to the next value and the value is returned by a Scan method. The Scan method can return an error.

Either use the Closer interface to stop/gracefully terminate something or allow an optional read-channel that the caller can close to trigger a graceful shutdown. Not receiving a Fetsh for a long time is not a sure indication that the interest is lost, not even for an ephemeral consumer

As I mentioned above already, I think having context for Fetch makes a ton of sense, since you (rightfully so) pointed out that now it is not possible to gracefully terminate Fetch() execution.

Let me know your thoughts on this.

[tpihl]

@piotrpio thank you for the long answer 👍

Fetch with context is absolutely not to my horror, since we can save server resources. Be my guest.

I think I understand a misunderstanding. Almost all my comments about "understand the code" refers to MY code, not the new jetstream interface. Contexts and errors, no issues in how to use. My concern is how it make my code look and if it makes my code harder to read and understand for others (or myself a few months down the line).

Every time the happy flow is interrupted by creating a context and deferring a call to it's cancel, it clutters/obfuscate what the code try to do. That is the same issue we still haven't solved for errors. Any call we make takes 3 or 4 lines when checking the error-result. But also making contexts add another 2 lines (with cancel either deferred or before checking error) making the actual call harder to find and understand.

The benefit of the extra code must exceed the higher cognitive load required to follow what the code does. (and I think that is also the most quoted reason for not implement any of the error-handling-abstractions discussed since forever).

I prefer to give each call the same amount of time to finish since I usually work event-based and focus on the lowest possible latency instead of a deadline covering multiple calls. Deadlines makes sense when synchronous...

So I understand your perspective to only handle time-out with a single mechanism. And I did read the iterator-discussion in the language repo. I understand that the bufio implementation have limitation and I would prefer for range over any other iterator implementation. My opinion is still that it makes my code to hard to understand unless I hide the new jetstream api in some abstraction of my own But it's fine, doing that allow me to chose what kind of limitation to accept where i callback or a msg on a channel is the way my code react to (or automatically retry ) a retry-able error

Thank you for listening to my opinion, I am sure we'll all learn to use it however it forms up in it's simplified way.

/T