fix: NatsJSConsumer never disposing receive activities

[aradalvand]

Fixes #910

Let me know if you don't think this is the right approach. The sub-optimal change here is that the newly-added ActivityEndingJSMsgReader is almost the duplicate of the already-existing ActivityEndingMsgReader, but I could not find a way to abstract over the both of them — not to mention they technically reside in different assemblies too (core and jetstream). (Update: Managed to consolidate).

[mtmk]

good catch, thanks @aradalvand

There are a few other implementations of NatsSubBase. Perhaps we should apply to all of them. We can take it step by step though no need to do all in one PR. wdyt?

[aradalvand]

@mtmk Great point! I just added it to NatsJSFetch and NatsJSOrderedConsume as well.
The other subclasses of NatsSubBase don't seem to expose a ChannelReader — for instance, NatsKVWatchSub takes in a Channel via its constructor and has a ReceiveAsync method like so:

nats.net/src/NATS.Client.KeyValueStore/Internal/NatsKVWatchSub.cs

Lines 51 to 59 in 29c5fb6

	protected override async ValueTask ReceiveInternalAsync(
	string subject,
	string? replyTo,
	ReadOnlySequence<byte>? headersBuffer,
	ReadOnlySequence<byte> payloadBuffer)
	{
	var msg = new NatsJSMsg<T>(NatsMsg<T>.Build(subject, replyTo, headersBuffer, payloadBuffer, _nats, _headerParser, _serializer), _context);
	await _commands.WriteAsync(new NatsKVWatchCommandMsg<T> { Command = NatsKVWatchCommand.Msg, Msg = msg }, _cancellationToken).ConfigureAwait(false);
	}

What do you think we should do for these?

[aradalvand]

In the last commit, I managed to merge ActivityEndingMsgReader and ActivityEndingJSMsgReader into a single class — in order to do this, I had to declare a shared interface for both INatsMsg<T> and INatsJSMsg<T> with a Headers property (called INatsMsg, which I feel is clean), and then making the original ActivityEndingMsgReader generic over any implementation of INatsMsg. Let me know what you think

[mtmk]

thanks @aradalvand looks good to me in general.

KV watchers are using push consumers so they are a little different. We can handle them later if needed.

The constrained reader using the new INatsMsg interface seems reasonable to me.

I know it might be tricky but we have a separate project just to test otle stuff. do you think there is a way we can test these changes there?

[aradalvand]

I know it might be tricky but we have a separate project just to test otle stuff. do you think there is a way we can test these changes there?

@mtmk I agree that it would be worthwhile to have a test suite for OTEL stuff — I haven't done that kind of thing myself, but I think it's probably doable by registering activity listeners and whatnot. I might look into it if I get the chance at some point. But good instincts there.

Though that's beyond the scope of this PR, so I just went ahead and created this issue #915

[mtmk]

LGTM thanks @aradalvand

could you merge 2.7 which has build fixes and run a 'dotnet format' to turn ci green please? we can merge right after that.

[aradalvand]

Done, sir. Though one Windows test still appears to be failing... which may not matter.