-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement nullability in streams Buffer #7496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Aaronontheweb
merged 6 commits into
akkadotnet:dev
from
Arkatufus:streams/cleanup-Buffer
Mar 18, 2025
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6fe8437
Implement nullability in streams Buffer
Arkatufus 0071443
Update API approval list
Arkatufus 34450c7
Merge branch 'dev' into streams/cleanup-Buffer
Arkatufus cf22168
Merge branch 'dev' into streams/cleanup-Buffer
Aaronontheweb de56304
post-merge cleanup
Arkatufus 19b2c65
Merge branch 'dev' into streams/cleanup-Buffer
Aaronontheweb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| using Akka.Event; | ||
| using Akka.Streams.Actors; | ||
|
|
||
| #nullable enable | ||
| namespace Akka.Streams.Implementation | ||
| { | ||
| /// <summary> | ||
|
|
@@ -40,7 +41,7 @@ public static Props Props(int bufferSize, OverflowStrategy overflowStrategy, Act | |
| /// <summary> | ||
| /// TBD | ||
| /// </summary> | ||
| protected readonly IBuffer<T> Buffer; | ||
| protected readonly IBuffer<T>? Buffer; | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
@@ -50,7 +51,6 @@ public static Props Props(int bufferSize, OverflowStrategy overflowStrategy, Act | |
| /// TBD | ||
| /// </summary> | ||
| public readonly OverflowStrategy OverflowStrategy; | ||
| private ILoggingAdapter _log; | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
@@ -63,13 +63,13 @@ public ActorRefSourceActor(int bufferSize, OverflowStrategy overflowStrategy, in | |
| { | ||
| BufferSize = bufferSize; | ||
| OverflowStrategy = overflowStrategy; | ||
| Buffer = bufferSize != 0 ? Implementation.Buffer.Create<T>(bufferSize, maxFixedBufferSize) : null; | ||
| Buffer = bufferSize > 0 ? Implementation.Buffer.Create<T>(bufferSize, maxFixedBufferSize) : null; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
| /// </summary> | ||
| protected ILoggingAdapter Log => _log ??= Context.GetLogger(); | ||
| protected ILoggingAdapter Log { get; } = Context.GetLogger(); | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
@@ -90,7 +90,7 @@ protected bool DefaultReceive(object message) | |
| Context.Stop(Self); | ||
| else if (message is Status.Success) | ||
| { | ||
| if (BufferSize == 0 || Buffer.IsEmpty) | ||
| if (Buffer is null || Buffer.IsEmpty) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM |
||
| OnCompleteThenStop(); // will complete the stream successfully | ||
| else | ||
| Context.Become(DrainBufferThenComplete); | ||
|
|
@@ -112,7 +112,7 @@ protected virtual bool RequestElement(object message) | |
| if (message is Request) | ||
| { | ||
| // totalDemand is tracked by base | ||
| if (BufferSize != 0) | ||
| if (Buffer is not null) | ||
| while (TotalDemand > 0L && !Buffer.IsEmpty) | ||
| OnNext(Buffer.Dequeue()); | ||
|
|
||
|
|
@@ -133,7 +133,7 @@ protected virtual bool ReceiveElement(T message) | |
| { | ||
| if (TotalDemand > 0L) | ||
| OnNext(message); | ||
| else if (BufferSize == 0) | ||
| else if (Buffer is null) | ||
| Log.Debug("Dropping element because there is no downstream demand: [{0}]", message); | ||
| else if (!Buffer.IsFull) | ||
| Buffer.Enqueue(message); | ||
|
|
@@ -189,7 +189,7 @@ private bool DrainBufferThenComplete(object message) | |
| // even if previously valid completion was requested via Status.Success | ||
| OnErrorThenStop(failure.Cause); | ||
| } | ||
| else if (message is Request) | ||
| else if (message is Request && Buffer is not null) | ||
| { | ||
| // totalDemand is tracked by base | ||
| while (TotalDemand > 0L && !Buffer.IsEmpty) | ||
|
|
@@ -201,7 +201,7 @@ private bool DrainBufferThenComplete(object message) | |
| else if (IsActive) | ||
| Log.Debug( | ||
| "Dropping element because Status.Success received already, only draining already buffered elements: [{0}] (pending: [{1}])", | ||
| message, Buffer.Used); | ||
| message, Buffer?.Used ?? 0); | ||
| else | ||
| return false; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| using System.Runtime.CompilerServices; | ||
| using Akka.Annotations; | ||
|
|
||
| #nullable enable | ||
| namespace Akka.Streams.Implementation | ||
| { | ||
| /// <summary> | ||
|
|
@@ -54,7 +55,7 @@ internal interface IBuffer<T> | |
| /// TBD | ||
| /// </summary> | ||
| /// <returns>TBD</returns> | ||
| T Peek(); | ||
| T? Peek(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Original behavior is that |
||
| /// <summary> | ||
| /// TBD | ||
| /// </summary> | ||
|
|
@@ -161,7 +162,7 @@ internal abstract class FixedSizeBuffer<T> : IBuffer<T> | |
| /// </summary> | ||
| protected long WriteIndex; | ||
|
|
||
| private readonly T[] _buffer; | ||
| private readonly T?[] _buffer; | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
@@ -222,14 +223,20 @@ public void Enqueue(T element) | |
| /// <param name="element">TBD</param> | ||
| /// <param name="maintenance">TBD</param> | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| public void Put(long index, T element, bool maintenance) => _buffer[ToOffset(index, maintenance)] = element; | ||
| public void Put(long index, T? element, bool maintenance) => _buffer[ToOffset(index, maintenance)] = element; | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
| /// </summary> | ||
| /// <param name="index">TBD</param> | ||
| /// <returns>TBD</returns> | ||
| public T Get(long index) => _buffer[ToOffset(index, false)]; | ||
| public T Get(long index) | ||
| { | ||
| var elem = _buffer[ToOffset(index, false)]; | ||
| if(elem == null) | ||
| throw new IndexOutOfRangeException($"Invalid buffer element at index {index}, element is null"); | ||
| return elem; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
@@ -354,7 +361,7 @@ private sealed class FixedQueue : IBuffer<T> | |
| private const int Size = 16; | ||
| private const int Mask = 15; | ||
|
|
||
| private readonly T[] _queue = new T[Size]; | ||
| private readonly T?[] _queue = new T[Size]; | ||
| private readonly BoundedBuffer<T> _boundedBuffer; | ||
| private int _head; | ||
| private int _tail; | ||
|
|
@@ -373,6 +380,9 @@ public FixedQueue(BoundedBuffer<T> boundedBuffer) | |
|
|
||
| public void Enqueue(T element) | ||
| { | ||
| if(element is null) | ||
| throw new ArgumentNullException(nameof(element)); | ||
|
|
||
| if (_tail - _head == Size) | ||
| { | ||
| var queue = new DynamicQueue(Capacity); | ||
|
|
@@ -392,12 +402,15 @@ public T Dequeue() | |
| { | ||
| var pos = _head & Mask; | ||
| var ret = _queue[pos]; | ||
| if(ret is null) | ||
| throw new IndexOutOfRangeException(); | ||
|
|
||
| _queue[pos] = default(T); | ||
| _head += 1; | ||
| return ret; | ||
| } | ||
|
|
||
| public T Peek() => _tail == _head ? default(T) : _queue[_head & Mask]; | ||
| public T? Peek() => _tail == _head ? default(T) : _queue[_head & Mask]; | ||
|
|
||
| public void Clear() | ||
| { | ||
|
|
@@ -431,12 +444,15 @@ public DynamicQueue(int capacity) | |
|
|
||
| public T Dequeue() | ||
| { | ||
| if(First is null) | ||
| throw new IndexOutOfRangeException(); | ||
|
|
||
| var result = First.Value; | ||
| RemoveFirst(); | ||
| return result; | ||
| } | ||
|
|
||
| public T Peek() => First.Value; | ||
| public T? Peek() => First is null ? default : First.Value; | ||
|
|
||
| public void DropHead() => RemoveFirst(); | ||
|
|
||
|
|
@@ -498,7 +514,7 @@ public BoundedBuffer(int capacity) | |
| /// TBD | ||
| /// </summary> | ||
| /// <returns>TBD</returns> | ||
| public T Peek() => _q.Peek(); | ||
| public T? Peek() => _q.Peek(); | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, should we throw an exception here if the
bufferSizeis less than0?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should, I just tried to preserve the old behavior