Wait until the subscription if finished before

Gsantomaggio · Gsantomaggio · commit 9edd363c5b0c · 2023-12-22T15:28:58.000+01:00
on the close and dispose call. If a disposed is called just after the creation
it waits.

* RawConsumer: Add check if the consumer is open before dispach
* Client: Add nextEntityId to have always a sequential ids. It avoids to use the same
  ids during the recycle
* ConnectionPool.FindNextValidId/2:Change the way to give the ids. Given an `nextid` in input
  the function will give always the next value

Signed-off-by: Gabriele Santomaggio &lt;G.santomaggio@gmail.com&gt;
diff --git a/RabbitMQ.Stream.Client/AbstractEntity.cs b/RabbitMQ.Stream.Client/AbstractEntity.cs
@@ -18,7 +18,8 @@ internal enum EntityStatus
     {
         Open,
         Closed,
-        Disposed
+        Disposed,
+        Initializing
     }
 
     public interface IClosable
@@ -64,7 +65,7 @@ private void MaybeCancelToken()
         /// <returns></returns>
         protected abstract Task<ResponseCode> DeleteEntityFromTheServer(bool ignoreIfAlreadyDeleted = false);
 
-        private readonly SemaphoreSlim _writeLock = new SemaphoreSlim(1, 1);
+        // private readonly SemaphoreSlim _writeLock = new SemaphoreSlim(1, 1);
 
         /// <summary>
         /// Internal close method. It is called by the public Close method.
@@ -76,35 +77,27 @@ private void MaybeCancelToken()
         /// <returns></returns>
         protected async Task<ResponseCode> Shutdown(EntityCommonConfig config, bool ignoreIfAlreadyDeleted = false)
         {
-            await _writeLock.WaitAsync().ConfigureAwait(false);
-            try
+            if (!IsOpen()) // the client is already closed
             {
-                MaybeCancelToken();
-
-                if (!IsOpen()) // the client is already closed
-                {
-                    return ResponseCode.Ok;
-                }
+                return ResponseCode.Ok;
+            }
 
-                _status = EntityStatus.Closed;
-                var result = await DeleteEntityFromTheServer(ignoreIfAlreadyDeleted).ConfigureAwait(false);
+            MaybeCancelToken();
 
-                if (_client is { IsClosed: true })
-                {
-                    return result;
-                }
+            _status = EntityStatus.Closed;
+            var result = await DeleteEntityFromTheServer(ignoreIfAlreadyDeleted).ConfigureAwait(false);
 
-                var closed = await _client.MaybeClose($"closing: {EntityId}",
-                        GetStream(), config.Pool)
-                    .ConfigureAwait(false);
-                ClientExceptions.MaybeThrowException(closed.ResponseCode, $"_client-close-Entity: {EntityId}");
-                Logger.LogDebug("{EntityInfo} is closed", DumpEntityConfiguration());
-                return result;
-            }
-            finally
+            if (_client is { IsClosed: true })
             {
-                _writeLock.Release();
+                return result;
             }
+
+            var closed = await _client.MaybeClose($"closing: {EntityId}",
+                    GetStream(), config.Pool)
+                .ConfigureAwait(false);
+            ClientExceptions.MaybeThrowException(closed.ResponseCode, $"_client-close-Entity: {EntityId}");
+            Logger.LogDebug("{EntityInfo} is closed", DumpEntityConfiguration());
+            return result;
         }
 
         protected void Dispose(bool disposing)
diff --git a/RabbitMQ.Stream.Client/Client.cs b/RabbitMQ.Stream.Client/Client.cs
@@ -207,6 +207,7 @@ public static async Task<Client> Create(ClientParameters parameters, ILogger log
             client.connection = await Connection
                 .Create(parameters.Endpoint, client.HandleIncoming, client.HandleClosed, parameters.Ssl, logger)
                 .ConfigureAwait(false);
+            client.connection.ClientId = client.ClientId;
             // exchange properties
             var peerPropertiesResponse = await client.Request<PeerPropertiesRequest, PeerPropertiesResponse>(corr =>
                 new PeerPropertiesRequest(corr, parameters.Properties)).ConfigureAwait(false);
@@ -306,7 +307,7 @@ public ValueTask<bool> Publish<T>(T msg) where T : struct, ICommand
             Action<(ulong, ResponseCode)[]> errorCallback, ConnectionsPool pool = null)
         {
             await _poolSemaphore.WaitAsync().ConfigureAwait(false);
-            var publisherId = ConnectionsPool.FindNextValidId(publishers.Keys.ToList());
+            var publisherId = ConnectionsPool.FindNextValidId(publishers.Keys.ToList(), IncrementEntityId());
             DeclarePublisherResponse response;
 
             try
@@ -320,7 +321,7 @@ public ValueTask<bool> Publish<T>(T msg) where T : struct, ICommand
                 _poolSemaphore.Release();
             }
 
-            if (response.ResponseCode == ResponseCode.Ok || pool == null)
+            if (response.ResponseCode == ResponseCode.Ok)
                 return (publisherId, response);
 
             // if the response code is not ok we need to remove the subscription
@@ -357,59 +358,65 @@ await Request<DeletePublisherRequest, DeletePublisherResponse>(corr =>
         public async Task<(byte, SubscribeResponse)> Subscribe(string stream, IOffsetType offsetType,
             ushort initialCredit,
             Dictionary<string, string> properties, Func<Deliver, Task> deliverHandler,
-            Func<bool, Task<IOffsetType>> consumerUpdateHandler = null)
+            Func<bool, Task<IOffsetType>> consumerUpdateHandler = null, ConnectionsPool pool = null)
         {
-            return await Subscribe(new RawConsumerConfig(stream) { OffsetSpec = offsetType },
+            return await Subscribe(new RawConsumerConfig(stream) { OffsetSpec = offsetType, Pool = pool },
                 initialCredit,
                 properties,
                 deliverHandler,
                 consumerUpdateHandler).ConfigureAwait(false);
         }
 
+        private byte _nextEntityId = 0;
+
+        // the entity id is a byte so we need to increment it and reset it when it reaches the max value
+        // to avoid to use always the same ids when producers and consumers are created 
+        // so even there is a connection with one producer or consumer we need to increment the id 
+        private byte IncrementEntityId()
+        {
+            lock (Obj)
+            {
+                var current = _nextEntityId;
+                _nextEntityId++;
+                if (_nextEntityId != byte.MaxValue)
+                    return current;
+                _nextEntityId = 0;
+                return _nextEntityId;
+            }
+        }
+
         public async Task<(byte, SubscribeResponse)> Subscribe(RawConsumerConfig config,
             ushort initialCredit,
             Dictionary<string, string> properties, Func<Deliver, Task> deliverHandler,
             Func<bool, Task<IOffsetType>> consumerUpdateHandler)
         {
             await _poolSemaphore.WaitAsync().ConfigureAwait(false);
-            var subscriptionId = ConnectionsPool.FindNextValidId(consumers.Keys.ToList());
-
+            var subscriptionId = ConnectionsPool.FindNextValidId(consumers.Keys.ToList(), IncrementEntityId());
+            SubscribeResponse response;
             try
             {
-                SubscribeResponse response;
-                try
-                {
-                    consumers.Add(subscriptionId,
-                        new ConsumerEvents(
-                            deliverHandler,
-                            consumerUpdateHandler));
-
-                    response = await Request<SubscribeRequest, SubscribeResponse>(corr =>
-                        new SubscribeRequest(corr, subscriptionId, config.Stream, config.OffsetSpec, initialCredit,
-                            properties)).ConfigureAwait(false);
-                }
-                finally
-                {
-                    _poolSemaphore.Release();
-                }
-
-                if (response.ResponseCode == ResponseCode.Ok)
-                    return (subscriptionId, response);
+                consumers.Add(subscriptionId,
+                    new ConsumerEvents(
+                        deliverHandler,
+                        consumerUpdateHandler));
 
-                ClientExceptions.MaybeThrowException(response.ResponseCode,
-                    $"Error while creating consumer for stream {config.Stream}");
+                response = await Request<SubscribeRequest, SubscribeResponse>(corr =>
+                    new SubscribeRequest(corr, subscriptionId, config.Stream, config.OffsetSpec, initialCredit,
+                        properties)).ConfigureAwait(false);
             }
-            catch (Exception e)
+            finally
             {
-                // if the response code is not ok we need to remove the subscription
-                // and close the connection if necessary. 
-                consumers.Remove(subscriptionId);
-                await MaybeClose("Create Consumer Exception", config.Stream, config.Pool).ConfigureAwait(false);
-                throw new CreateConsumerException(
-                    $"Error while creating consumer for stream {config.Stream}, error: {e.Message}");
+                _poolSemaphore.Release();
             }
 
-            return (subscriptionId, new SubscribeResponse(subscriptionId, ResponseCode.InternalError));
+            if (response.ResponseCode == ResponseCode.Ok)
+                return (subscriptionId, response);
+
+            // if the response code is not ok we need to remove the subscription
+            // and close the connection if necessary. 
+            consumers.Remove(subscriptionId);
+            await MaybeClose("Create Consumer Exception", config.Stream, config.Pool).ConfigureAwait(false);
+            return (subscriptionId, response);
         }
 
         public async Task<UnsubscribeResponse> Unsubscribe(byte subscriptionId, bool ignoreIfAlreadyRemoved = false)
@@ -799,7 +806,6 @@ internal async Task<CloseResponse> MaybeClose(string reason, string stream, Conn
                         _logger.LogInformation("Close connection for the {ClientId}", ClientId);
                         // the client can be closed in an unexpected way so we need to remove it from the pool
                         // so you will find pool.remove(ClientId) also to the disconnect event
-                        // pool.remove(ClientId) is a duplicate call here but it is ok. The pool is idempotent 
                         pool.Remove(ClientId);
                         await Close(reason).ConfigureAwait(false);
                     }
diff --git a/RabbitMQ.Stream.Client/Connection.cs b/RabbitMQ.Stream.Client/Connection.cs
@@ -33,7 +33,7 @@ public class Connection : IDisposable
         private CancellationToken Token => _cancelTokenSource.Token;
 
         internal int NumFrames => numFrames;
-
+        internal string ClientId { get; set; }
         public bool IsClosed => isClosed;
 
         private static System.IO.Stream MaybeTcpUpgrade(NetworkStream networkStream, SslOption sslOption)
@@ -191,6 +191,8 @@ private async Task ProcessIncomingFrames()
             finally
             {
                 isClosed = true;
+                _logger?.LogDebug("TCP Connection Closed ClientId: {ClientId} is IsCancellationRequested {Token} ",
+                    ClientId, Token.IsCancellationRequested);
                 // Mark the PipeReader as complete
                 await reader.CompleteAsync(caught).ConfigureAwait(false);
                 var t = closedCallback?.Invoke("TCP Connection Closed")!;
diff --git a/RabbitMQ.Stream.Client/ConnectionsPool.cs b/RabbitMQ.Stream.Client/ConnectionsPool.cs
@@ -100,30 +100,35 @@ public class ConnectionsPool
 {
     private static readonly object s_lock = new();
 
-    internal static byte FindNextValidId(List<byte> ids)
+    internal static byte FindNextValidId(List<byte> ids, byte nextId = 0)
     {
         lock (s_lock)
         {
-            if (ids.Count == 0)
-            {
-                return 0;
-            }
-
             // // we start with the recycle when we reach the max value
             // // in this way we can avoid to recycle the same ids in a short time
             ids.Sort();
-            if (ids[^1] != byte.MaxValue)
-                return (byte)(ids[^1] + 1);
+            var l = ids.Where(b => b >= nextId).ToList();
+            l.Sort();
+            if (l.Count == 0)
+            {
+                // not necessary to start from 0 because the ids are recycled
+                // nextid is passed as parameter to avoid to start from 0
+                // see client:IncrementEntityId/0
+                return nextId;
+            }
+
+            if (l[^1] != byte.MaxValue)
+                return (byte)(l[^1] + 1);
 
             for (var i = 0; i < ids.Count - 1; i++)
             {
-                if (ids[i + 1] - ids[i] > 1)
+                if (l[i + 1] - l[i] > 1)
                 {
-                    return (byte)(ids[i] + 1);
+                    return (byte)(l[i] + 1);
                 }
             }
 
-            return (byte)(ids[^1] + 1);
+            return (byte)(l[^1] + 1);
         }
     }
 
diff --git a/RabbitMQ.Stream.Client/PublicAPI.Shipped.txt b/RabbitMQ.Stream.Client/PublicAPI.Shipped.txt
@@ -186,7 +186,6 @@ RabbitMQ.Stream.Client.Client.QueryPublisherSequence(string publisherRef, string
 RabbitMQ.Stream.Client.Client.StoreOffset(string reference, string stream, ulong offsetValue) -> System.Threading.Tasks.ValueTask<bool>
 RabbitMQ.Stream.Client.Client.StreamExists(string stream) -> System.Threading.Tasks.Task<bool>
 RabbitMQ.Stream.Client.Client.Subscribe(RabbitMQ.Stream.Client.RawConsumerConfig config, ushort initialCredit, System.Collections.Generic.Dictionary<string, string> properties, System.Func<RabbitMQ.Stream.Client.Deliver, System.Threading.Tasks.Task> deliverHandler, System.Func<bool, System.Threading.Tasks.Task<RabbitMQ.Stream.Client.IOffsetType>> consumerUpdateHandler) -> System.Threading.Tasks.Task<(byte, RabbitMQ.Stream.Client.SubscribeResponse)>
-RabbitMQ.Stream.Client.Client.Subscribe(string stream, RabbitMQ.Stream.Client.IOffsetType offsetType, ushort initialCredit, System.Collections.Generic.Dictionary<string, string> properties, System.Func<RabbitMQ.Stream.Client.Deliver, System.Threading.Tasks.Task> deliverHandler, System.Func<bool, System.Threading.Tasks.Task<RabbitMQ.Stream.Client.IOffsetType>> consumerUpdateHandler = null) -> System.Threading.Tasks.Task<(byte, RabbitMQ.Stream.Client.SubscribeResponse)>
 RabbitMQ.Stream.Client.ClientParameters
 RabbitMQ.Stream.Client.ClientParameters.AddressResolver.get -> RabbitMQ.Stream.Client.AddressResolver
 RabbitMQ.Stream.Client.ClientParameters.AddressResolver.set -> void
diff --git a/RabbitMQ.Stream.Client/PublicAPI.Unshipped.txt b/RabbitMQ.Stream.Client/PublicAPI.Unshipped.txt
@@ -33,6 +33,7 @@ RabbitMQ.Stream.Client.Client.DeletePublisher(byte publisherId, bool ignoreIfAlr
 RabbitMQ.Stream.Client.Client.ExchangeVersions() -> System.Threading.Tasks.Task<RabbitMQ.Stream.Client.CommandVersionsResponse>
 RabbitMQ.Stream.Client.Client.QueryRoute(string superStream, string routingKey) -> System.Threading.Tasks.Task<RabbitMQ.Stream.Client.RouteQueryResponse>
 RabbitMQ.Stream.Client.Client.StreamStats(string stream) -> System.Threading.Tasks.ValueTask<RabbitMQ.Stream.Client.StreamStatsResponse>
+RabbitMQ.Stream.Client.Client.Subscribe(string stream, RabbitMQ.Stream.Client.IOffsetType offsetType, ushort initialCredit, System.Collections.Generic.Dictionary<string, string> properties, System.Func<RabbitMQ.Stream.Client.Deliver, System.Threading.Tasks.Task> deliverHandler, System.Func<bool, System.Threading.Tasks.Task<RabbitMQ.Stream.Client.IOffsetType>> consumerUpdateHandler = null, RabbitMQ.Stream.Client.ConnectionsPool pool = null) -> System.Threading.Tasks.Task<(byte, RabbitMQ.Stream.Client.SubscribeResponse)>
 RabbitMQ.Stream.Client.Client.Unsubscribe(byte subscriptionId, bool ignoreIfAlreadyRemoved = false) -> System.Threading.Tasks.Task<RabbitMQ.Stream.Client.UnsubscribeResponse>
 RabbitMQ.Stream.Client.ClientParameters.AuthMechanism.get -> RabbitMQ.Stream.Client.AuthMechanism
 RabbitMQ.Stream.Client.ClientParameters.AuthMechanism.set -> void
diff --git a/RabbitMQ.Stream.Client/RawConsumer.cs b/RabbitMQ.Stream.Client/RawConsumer.cs
@@ -266,7 +266,20 @@ async Task DispatchMessage(Message message, ulong i)
                                 // it is useful only in single active consumer
                                 if (IsPromotedAsActive)
                                 {
-                                    var canDispatch = true;
+                                    if (_status != EntityStatus.Open)
+                                    {
+                                        Logger?.LogDebug(
+                                            "{EntityInfo} is not active. message won't dispatched",
+                                            DumpEntityConfiguration());
+                                    }
+
+                                    // can dispatch only if the consumer is active
+                                    // it usually at this point the consumer is active
+                                    // but in rare case where the consumer is closed and open in a short
+                                    // time the ids could be the same to not problem we need just to skip the message
+                                    // Given the way how the ids are generated it is very rare to have the same ids
+                                    // it is just a safety check
+                                    var canDispatch = _status == EntityStatus.Open;
 
                                     if (_config.IsFiltering)
                                     {
@@ -280,7 +293,7 @@ async Task DispatchMessage(Message message, ulong i)
                                         }
                                         catch (Exception e)
                                         {
-                                            Logger.LogError(e,
+                                            Logger?.LogError(e,
                                                 "Error while filtering message. Message  with offset {MessageOffset} won't be dispatched."
                                                 + "Suggestion: review the PostFilter value function"
                                                 + "{EntityInfo}",
@@ -394,7 +407,7 @@ private void ProcessChunks()
             {
                 // need to wait the subscription is completed 
                 // else the _subscriberId could be incorrect
-                await _completeSubscription.Task.ConfigureAwait(false);
+                _completeSubscription.Task.Wait();
                 try
                 {
                     while (!Token.IsCancellationRequested &&
@@ -500,6 +513,7 @@ private async Task Init()
             var chunkConsumed = 0;
             // this the default value for the consumer.
             _config.StoredOffsetSpec = _config.OffsetSpec;
+            _status = EntityStatus.Initializing;
             (EntityId, var response) = await _client.Subscribe(
                 _config,
                 _initialCredits,
@@ -580,8 +594,9 @@ private async Task Init()
 
             if (response.ResponseCode == ResponseCode.Ok)
             {
-                _completeSubscription.SetResult();
                 _status = EntityStatus.Open;
+                // the subscription is completed so the parsechunk can start to process the chunks
+                _completeSubscription.SetResult();
                 return;
             }
 
@@ -653,12 +668,13 @@ public override async Task<ResponseCode> Close()
             // when the consumer is closed we must be sure that the 
             // the subscription is completed to avoid problems with the connection
             // It could happen when the closing is called just after the creation
-            await _completeSubscription.Task.ConfigureAwait(false);
+            _completeSubscription.Task.Wait();
             return await Shutdown(_config).ConfigureAwait(false);
         }
 
         public void Dispose()
         {
+            _completeSubscription.Task.Wait();
             try
             {
                 Dispose(true);
diff --git a/Tests/ClientTests.cs b/Tests/ClientTests.cs
diff --git a/Tests/ConnectionsPoolTests.cs b/Tests/ConnectionsPoolTests.cs

Original file line number	Diff line number	Diff line change
`@@ -100,30 +100,35 @@ public class ConnectionsPool`
`100`	`100`	`{`
`101`	`101`	`private static readonly object s_lock = new();`
`102`	`102`
`103`		`- internal static byte FindNextValidId(List<byte> ids)`
	`103`	`+ internal static byte FindNextValidId(List<byte> ids, byte nextId = 0)`
`104`	`104`	`{`
`105`	`105`	`lock (s_lock)`
`106`	`106`	`{`
`107`		`- if (ids.Count == 0)`
`108`		`- {`
`109`		`- return 0;`
`110`		`- }`
`111`		`-`
`112`	`107`	`// // we start with the recycle when we reach the max value`
`113`	`108`	`// // in this way we can avoid to recycle the same ids in a short time`
`114`	`109`	`ids.Sort();`
`115`		`- if (ids[^1] != byte.MaxValue)`
`116`		`- return (byte)(ids[^1] + 1);`
	`110`	`+ var l = ids.Where(b => b >= nextId).ToList();`
	`111`	`+ l.Sort();`
	`112`	`+ if (l.Count == 0)`
	`113`	`+ {`
	`114`	`+ // not necessary to start from 0 because the ids are recycled`
	`115`	`+ // nextid is passed as parameter to avoid to start from 0`
	`116`	`+ // see client:IncrementEntityId/0`
	`117`	`+ return nextId;`
	`118`	`+ }`
	`119`	`+`
	`120`	`+ if (l[^1] != byte.MaxValue)`
	`121`	`+ return (byte)(l[^1] + 1);`
`117`	`122`
`118`	`123`	`for (var i = 0; i < ids.Count - 1; i++)`
`119`	`124`	`{`
`120`		`- if (ids[i + 1] - ids[i] > 1)`
	`125`	`+ if (l[i + 1] - l[i] > 1)`
`121`	`126`	`{`
`122`		`- return (byte)(ids[i] + 1);`
	`127`	`+ return (byte)(l[i] + 1);`
`123`	`128`	`}`
`124`	`129`	`}`
`125`	`130`
`126`		`- return (byte)(ids[^1] + 1);`
	`131`	`+ return (byte)(l[^1] + 1);`
`127`	`132`	`}`
`128`	`133`	`}`
`129`	`134`