Release 0.14.0 #24
Release 0.14.0 #24
Conversation
- Added *PublishSafe* method to *IMessageBrokerService* to allow publishing messages safely in chase of chain subscriptions during publishing of a message **Changed**: - *Subscribe* and *Unsubscribe* throw an *InvalidOperationException* when being executed during a message being published **Fixed**: - CoroutineTests issues running when building released projects
|
Warning CodeRabbit GitHub Action detectedThe repository is using both CodeRabbit Pro and CodeRabbit Open Source (via GitHub Actions), which is not recommended as it may lead to duplicate comments and extra noise. Please remove the CodeRabbit GitHub Action. Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces version 0.14.0 of the software, updating the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
WalkthroughThe changes in this pull request involve the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (5)
- CHANGELOG.md (1)
- Runtime/MessageBrokerService.cs (5)
- Tests/Editor/EditMode/InstallerTest.cs (1)
- Tests/Editor/EditMode/MessageBrokerServiceTest.cs (5)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (2)
Files ignored due to filter (2)
- Tests/Editor/EditMode/GameLovers.Services.Tests.asmdef.meta
- package.json
Files skipped from review due to trivial changes (1)
- Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef
Review comments generated (13)
- Review: 9
- LGTM: 4
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -1,9 +1,10 @@ | |||
| using System; | |||
| using GameLovers.Services; | |||
There was a problem hiding this comment.
The using directive for GameLovers.Services has been removed and replaced with a new using directive for GameLoversEditor.Services.Tests. Please verify if this change is intentional and aligns with the desired namespace for the test class.
- using GameLovers.Services;
+ using GameLoversEditor.Services.Tests;| _messageBroker.PublishSafe(_messageType1); | ||
|
|
||
| _subscriber.Received().MockMessageCall(_messageType1); | ||
| _subscriber.Received(2).MockMessageCall(_messageType1); | ||
| } | ||
|
|
||
| [Test] |
There was a problem hiding this comment.
The addition of the PublishSafe method to the IMessageBrokerService interface and the modification of the behavior of the Subscribe and Unsubscribe methods seem appropriate. However, it would be helpful to have more context about the changes made in these methods.
| _messageBroker.PublishSafe(_messageType1); | ||
|
|
There was a problem hiding this comment.
Consider adding a comment or documentation to explain the purpose and behavior of the PublishSafe method.
| public void Subscribe_MultipleSubscriptionSameType_ReplacePreviousSubscription() | ||
| { | ||
| _messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall); | ||
| _messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2); | ||
| _messageBroker.Publish(_messageType1); | ||
| _messageBroker.PublishSafe(_messageType1); | ||
|
|
||
| _subscriber.DidNotReceive().MockMessageCall(_messageType1); | ||
| _subscriber.Received(2).MockMessageCall2(_messageType1); | ||
| } |
There was a problem hiding this comment.
The changes made to test multiple subscriptions and chain subscriptions are reasonable. However, it would be beneficial to have more information about the specific issues related to CoroutineTests that were fixed.
| public void Publish_ChainSubscribe_Successfully() | ||
| { | ||
| // TODO: Test | ||
| Assert.True(true); | ||
| } |
There was a problem hiding this comment.
The placeholder test method Publish_ChainSubscribe_Successfully does not contain any assertions or meaningful test logic. It should either be removed or implemented with relevant test cases.
| _messageBroker.Unsubscribe<MessageType1>(_subscriber); | ||
| _messageBroker.Publish(_messageType1); | ||
| _messageBroker.PublishSafe(_messageType1); | ||
|
|
||
| _subscriber.DidNotReceive().MockMessageCall(_messageType1); | ||
| } |
There was a problem hiding this comment.
The changes made to test the successful unsubscribe operation and removing all subscriptions of the same type are appropriate.
| public void UnsubscribeWithAction_MultipleSubscriptionSameType_RemoveAllScriptionsOfSameType() | ||
| { | ||
| _messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall); | ||
| _messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2); | ||
| _messageBroker.Unsubscribe<MessageType1>(_subscriber.MockMessageCall); | ||
| _messageBroker.Unsubscribe<MessageType1>(_subscriber); | ||
| _messageBroker.Publish(_messageType1); | ||
| _messageBroker.PublishSafe(_messageType1); | ||
|
|
||
| _subscriber.DidNotReceive().MockMessageCall(_messageType1); | ||
| _subscriber.Received().MockMessageCall2(_messageType1); | ||
| _subscriber.DidNotReceive().MockMessageCall2(_messageType1); |
There was a problem hiding this comment.
The modifications to test the behavior of unsubscribing with multiple subscriptions of the same type and publishing messages after unsubscribing are valid.
| _messageBroker.PublishSafe(_messageType2); | ||
|
|
||
| _subscriber.DidNotReceive().MockMessageCall(_messageType1); | ||
| _subscriber.Received().MockMessageAlternativeCall(_messageType2); | ||
| _subscriber.Received(2).MockMessageAlternativeCall(_messageType2); |
There was a problem hiding this comment.
The changes made to test unsubscribing from one message type while still subscribing to another message type are reasonable.
| _messageBroker.Publish(_messageType1); | ||
| _messageBroker.Publish(_messageType2); | ||
| _messageBroker.Publish(_messageType2); | ||
| _messageBroker.PublishSafe(_messageType1); | ||
| _messageBroker.PublishSafe(_messageType2); |
There was a problem hiding this comment.
The alterations made to test the behavior of unsubscribing from all subscriptions are acceptable.
133:
The addition of the test case to verify that unsubscribing without any subscriptions does nothing is appropriate.
Overall, the changes made to the MessageBrokerServiceTest.cs file seem reasonable and align with the added functionality and modifications described in the PR summary.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (10)
Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (1)
17-19: LGTM! Proper test isolation with UNITY_INCLUDE_TESTS.Adding the UNITY_INCLUDE_TESTS define constraint ensures that test assemblies are only included in test builds, which is crucial for:
- Preventing test code from being included in release builds
- Addressing the CoroutineTests issues mentioned in the PR objectives
This is a Unity best practice that helps maintain a clean separation between test and production code.
Tests/Editor/EditMode/MessageBrokerServiceTest.cs (2)
46-56: LGTM: Well-structured test for subscription replacementThe test effectively verifies that new subscriptions replace previous ones for the same message type. Consider renaming to "Subscribe_MultipleSubscriptionsSameType_ReplacePreviousSubscription" (plural 'Subscriptions') for consistency with other test names.
86-95: Fix typo in test nameThe test name contains a typo: "Scriptions" should be "Subscriptions". The correct name should be "UnsubscribeWithAction_MultipleSubscriptionSameType_RemoveAllSubscriptionsOfSameType".
-public void UnsubscribeWithAction_MultipleSubscriptionSameType_RemoveAllScriptionsOfSameType() +public void UnsubscribeWithAction_MultipleSubscriptionSameType_RemoveAllSubscriptionsOfSameType()CHANGELOG.md (2)
10-10: Fix typo in new features sectionThere's a typo in the word "chase" which should be "case".
-to allow publishing messages safely in chase of chain subscriptions during publishing of a message +to allow publishing messages safely in case of chain subscriptions during publishing of a message
16-16: Consider adding more details about the CoroutineTests fixThe fix description "CoroutineTests issues running when building released projects" could be more specific about what issues were fixed and how they were resolved. This would help with future debugging and provide better context for the changes.
Consider expanding with details like:
- What specific issues were encountered?
- How were they fixed?
- What impact did this have on released builds?
Runtime/MessageBrokerService.cs (5)
29-29: Typo in XML documentation comment.In line 29, the word "it" should be "if":
-/// Use <see cref="PublishSafe{T}(T)"/> it there are chain subscriptions during publishing +/// Use <see cref="PublishSafe{T}(T)"/> if there are chain subscriptions during publishing
67-67: Potential issues with_isPublishingimplementation.Using a
(bool, IMessage)tuple for_isPublishingis not descriptive and may lead to confusion. Consider creating a dedicated class or struct to represent the publishing state for better code clarity.
Line range hint
65-157: Overall lack of thread safety inMessageBrokerService.The class does not use synchronization mechanisms to handle concurrent access to
_subscriptionsand_isPublishing. In a multi-threaded application, this could lead to race conditions and data corruption. Consider the following refactor:
- Use
ConcurrentDictionaryfor_subscriptionsto handle concurrent reads and writes safely.- Implement locking (e.g.,
lockstatements) around critical sections where shared state is modified.- Ensure that all access to shared fields like
_isPublishingis thread-safe.Example of using
ConcurrentDictionary:private readonly ConcurrentDictionary<Type, ConcurrentDictionary<object, Delegate>> _subscriptions = new(); private readonly object _publishLock = new(); public void Publish<T>(T message) where T : IMessage { ConcurrentDictionary<object, Delegate> subscriptionObjects; if (!_subscriptions.TryGetValue(typeof(T), out subscriptionObjects)) { return; } Delegate[] subscribers; lock (_publishLock) { subscribers = subscriptionObjects.Values.ToArray(); } foreach (var subscriber in subscribers) { ((Action<T>)subscriber)(message); } }
41-41: Missing implementation details in XML documentation forPublishSafe<T>.The summary and remarks provide good high-level information but lack specifics about the method's behavior in concurrent scenarios. Consider adding details about thread safety and how
PublishSafehandles chain subscriptions to guide users effectively.
38-39: Grammar correction in XML documentation.In line 38, "allocated" should be "allocate":
-/// This method can be slow and allocated extra memory if there are a lot of subscribers to the <typeparamref name="T"/>. +/// This method can be slow and allocate extra memory if there are a lot of subscribers to the <typeparamref name="T"/>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)Runtime/MessageBrokerService.cs(5 hunks)Tests/Editor/EditMode/InstallerTest.cs(1 hunks)Tests/Editor/EditMode/MessageBrokerServiceTest.cs(5 hunks)Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef(2 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Tests/Editor/EditMode/InstallerTest.cs
- package.json
🔇 Additional comments (7)
Tests/Editor/PlayMode/GameLovers.Services.Tests.Playmode.asmdef (1)
5-7: LGTM! Improved reference clarity by using direct package references.
The change from GUID references to direct package references makes the dependencies more explicit and easier to maintain.
Tests/Editor/EditMode/MessageBrokerServiceTest.cs (4)
41-43: LGTM: Test updated correctly for PublishSafe
The test has been properly updated to verify both Publish and PublishSafe methods with the correct assertion count.
69-71: LGTM: Proper verification of no-subscription scenario
Test correctly verifies that both Publish and PublishSafe methods handle the no-subscription scenario appropriately.
78-80: LGTM: Comprehensive unsubscribe verification
Test properly verifies that unsubscribed handlers don't receive messages from either Publish or PublishSafe methods.
105-108: LGTM: Proper verification of type-specific unsubscribe
Test correctly verifies that unsubscribing one message type doesn't affect subscriptions of different types, for both Publish and PublishSafe methods.
Runtime/MessageBrokerService.cs (2)
33-41: Addition of PublishSafe<T>(T message) method is well-documented and appropriate.
The new PublishSafe method is correctly declared and includes comprehensive XML documentation, explaining its purpose and usage scenarios.
50-53: Update to Unsubscribe<T> method signature enhances flexibility.
Changing the parameter to object subscriber = null allows for unsubscribing all actions associated with a specific subscriber or all subscribers if null is provided. The XML comments accurately reflect this behavior.
| [Test] | ||
| public void Publish_ChainSubscribe_Successfully() | ||
| { | ||
| // TODO: Test | ||
| Assert.True(true); |
There was a problem hiding this comment.
Critical: Missing test implementation for chain subscriptions
This test is crucial as chain subscriptions are a key feature mentioned in the PR objectives, specifically for the new PublishSafe method. The current placeholder doesn't verify this important functionality.
Would you like me to help implement this test? Here's what we should verify:
- Chain subscription scenario where publishing a message triggers another subscription
- Verify that PublishSafe handles this scenario correctly
- Compare behavior between Publish and PublishSafe methods in chain subscription scenarios
| Assert.DoesNotThrow(() => _messageBroker.Unsubscribe<MessageType1>(_subscriber)); | ||
| Assert.DoesNotThrow(() => _messageBroker.Unsubscribe<MessageType1>()); | ||
| Assert.DoesNotThrow(() => _messageBroker.UnsubscribeAll()); | ||
| } |
There was a problem hiding this comment.
Missing test cases for InvalidOperationException scenarios
According to the PR objectives, Subscribe and Unsubscribe methods should throw InvalidOperationException when called during message publishing. These scenarios are not covered by the current test suite.
Would you like me to help implement additional test cases to verify:
- Subscribe throws InvalidOperationException when called during message publishing
- Unsubscribe throws InvalidOperationException when called during message publishing
There was a problem hiding this comment.
give me the test cases for both scenearios
There was a problem hiding this comment.
@CoderGamester, here are the test cases for both scenarios:
[Test]
public void Subscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2);
});
});
_messageBroker.Publish(_messageType1);
}
[Test]
public void Unsubscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Unsubscribe<MessageType1>(_subscriber);
});
});
_messageBroker.Publish(_messageType1);
}These tests verify that Subscribe and Unsubscribe throw an InvalidOperationException when called during message publishing, as specified in the PR objectives.
There was a problem hiding this comment.
@coderabbitai give me the test cases for both scenarios
There was a problem hiding this comment.
CoderGamester: Here are the test cases for both scenarios:
[Test]
public void Subscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2);
});
});
_messageBroker.Publish(_messageType1);
}
[Test]
public void Unsubscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Unsubscribe<MessageType1>(_subscriber);
});
});
_messageBroker.Publish(_messageType1);
}These tests verify that Subscribe and Unsubscribe throw an InvalidOperationException when called during message publishing, as specified in the PR objectives.
There was a problem hiding this comment.
@CoderGamester, here are the test cases for both scenarios:
[Test]
public void Subscribe_DuringPublish_ThrowsInvalidOperationException()
{
// Arrange
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall);
// Set up the subscriber to attempt to subscribe during publishing
_subscriber.When(s => s.MockMessageCall(_messageType1)).Do(_ =>
{
// Act & Assert
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2);
});
});
// Act
_messageBroker.Publish(_messageType1);
// Verify that the second subscriber was not added
_messageBroker.Publish(_messageType1);
_subscriber.DidNotReceive().MockMessageCall2(_messageType1);
}
[Test]
public void Unsubscribe_DuringPublish_ThrowsInvalidOperationException()
{
// Arrange
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall);
// Set up the subscriber to attempt to unsubscribe during publishing
_subscriber.When(s => s.MockMessageCall(_messageType1)).Do(_ =>
{
// Act & Assert
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Unsubscribe<MessageType1>(_subscriber);
});
});
// Act
_messageBroker.Publish(_messageType1);
// Verify that the subscriber is still subscribed
_messageBroker.Publish(_messageType1);
_subscriber.Received(2).MockMessageCall(_messageType1);
}These tests verify that Subscribe and Unsubscribe throw an InvalidOperationException when called during message publishing, as specified in the PR objectives.
Let me know if you need any further assistance!
There was a problem hiding this comment.
CoderGamester: Here are the test cases for both scenarios:
[Test]
public void Subscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2);
});
});
_messageBroker.Publish(_messageType1);
}
[Test]
public void Unsubscribe_DuringPublish_ThrowsInvalidOperationException()
{
_messageBroker.Subscribe<MessageType1>(message =>
{
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Unsubscribe<MessageType1>(_subscriber);
});
});
_messageBroker.Publish(_messageType1);
}These tests verify that Subscribe and Unsubscribe throw an InvalidOperationException when called during message publishing, as specified in the PR objectives. Let me know if you need anything else!
There was a problem hiding this comment.
@CoderGamester, here are the test cases for both scenarios:
[Test]
public void Subscribe_DuringPublish_ThrowsInvalidOperationException()
{
// Arrange
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall);
// Act
_subscriber.When(s => s.MockMessageCall(_messageType1)).Do(callInfo =>
{
// Assert
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall2);
});
});
_messageBroker.Publish(_messageType1);
}
[Test]
public void Unsubscribe_DuringPublish_ThrowsInvalidOperationException()
{
// Arrange
_messageBroker.Subscribe<MessageType1>(_subscriber.MockMessageCall);
// Act
_subscriber.When(s => s.MockMessageCall(_messageType1)).Do(callInfo =>
{
// Assert
Assert.Throws<InvalidOperationException>(() =>
{
_messageBroker.Unsubscribe<MessageType1>(_subscriber);
});
});
_messageBroker.Publish(_messageType1);
}These test cases verify that Subscribe and Unsubscribe throw an InvalidOperationException when called during message publishing, as specified in the PR objectives.
Let me know if you need anything else!
There was a problem hiding this comment.
@coderabbitai, thank you for providing the test cases. They look good and cover the scenarios where Subscribe and Unsubscribe methods should throw InvalidOperationException when called during message publishing.
I will implement these test cases in the code.
There was a problem hiding this comment.
@coderabbitai, thank you for providing the test cases. They look good and cover the scenarios where Subscribe and Unsubscribe methods should throw InvalidOperationException when called during message publishing.
I will implement these test cases in the code.
Runtime/MessageBrokerService.cs
Outdated
| public class MessageBrokerService : IMessageBrokerService | ||
| { | ||
| private readonly IDictionary<Type, IDictionary<object, IList>> _subscriptions = new Dictionary<Type, IDictionary<object, IList>>(); | ||
| private readonly IDictionary<Type, IDictionary<object, object>> _subscriptions = new Dictionary<Type, IDictionary<object, object>>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use of object in _subscriptions dictionary reduces type safety.
Changing _subscriptions to IDictionary<Type, IDictionary<object, object>> with keys and values as object can lead to runtime errors and makes the code harder to maintain. Consider using more specific types to enhance type safety. For example:
private readonly IDictionary<Type, IDictionary<object, Delegate>> _subscriptions = new Dictionary<Type, IDictionary<object, Delegate>>();This change uses Delegate to store delegates, which is more specific than object.
| _isPublishing = (true, message); | ||
|
|
There was a problem hiding this comment.
Thread-safety concerns with _isPublishing field in Publish<T> method.
The _isPublishing field is accessed without any synchronization, which can cause race conditions in a multi-threaded environment. If multiple threads call Publish simultaneously, the state of _isPublishing may become inconsistent. Consider implementing thread synchronization mechanisms to protect shared state.
Also applies to: 86-87
| public void PublishSafe<T>(T message) where T : IMessage | ||
| { | ||
| if (!_subscriptions.TryGetValue(typeof(T), out var subscriptionObjects)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var subscriptionCopy = new object[subscriptionObjects.Count]; | ||
|
|
||
| subscriptionObjects.Values.CopyTo(subscriptionCopy, 0); | ||
|
|
||
| for (var i = 0; i < subscriptionCopy.Length; i++) | ||
| { | ||
| var actions = (List<Action<T>>) subscriptionCopy[i]; | ||
| var action = (Action<T>)subscriptionCopy[i]; | ||
|
|
||
| for (var index = 0; index < actions.Count; index++) | ||
| { | ||
| actions[index](message); | ||
| } | ||
| action(message); |
There was a problem hiding this comment.
Ensure thread safety in PublishSafe<T>(T message) method.
While PublishSafe copies the subscription list to handle modifications during publishing, it doesn't protect against concurrent modifications from other threads. If the subscription dictionary is modified by another thread during the copy operation, it could throw an exception. Consider using thread-safe collections or locking mechanisms.
| if(_isPublishing.Item1) | ||
| { | ||
| subscriptionObjects = new Dictionary<object, IList>(); | ||
| _subscriptions.Add(type, subscriptionObjects); | ||
| throw new InvalidOperationException($"Cannot subscribe to {type.Name} message while publishing " + | ||
| $"{_isPublishing.Item2.GetType().Name} message. Use {nameof(PublishSafe)} instead!"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exception handling in Subscribe<T> may lead to unexpected behavior.
Throwing an InvalidOperationException when subscribing during publishing may disrupt the application's flow, especially if the caller is not expecting it. Additionally, without thread synchronization, this check may not reliably prevent issues. Consider handling this scenario more gracefully or documenting the behavior clearly.
| if (_isPublishing.Item1) | ||
| { | ||
| subscriptionObjects.Remove(subscriber); | ||
| throw new InvalidOperationException($"Cannot unsubscribe to {type.Name} message while publishing " + | ||
| $"{_isPublishing.Item2.GetType().Name} message. Use {nameof(PublishSafe)} instead!"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exception handling in Unsubscribe<T> may cause application instability.
Similar to Subscribe<T>, throwing an exception when unsubscribing during publishing could lead to unexpected crashes. Without proper synchronization, the _isPublishing check may not be reliable. Consider a thread-safe approach to handle unsubscriptions without raising exceptions.
| throw new InvalidOperationException($"Cannot subscribe to {type.Name} message while publishing " + | ||
| $"{_isPublishing.Item2.GetType().Name} message. Use {nameof(PublishSafe)} instead!"); |
There was a problem hiding this comment.
Potential NullReferenceException in exception messages.
If _isPublishing.Item2 is null, calling _isPublishing.Item2.GetType().Name will throw a NullReferenceException. Ensure that message is not null when setting _isPublishing, or add a null check before accessing GetType().
Apply this diff to add null checks:
+if (_isPublishing.Item2 != null)
+{
throw new InvalidOperationException($"Cannot subscribe to {type.Name} message while publishing " +
$"{_isPublishing.Item2.GetType().Name} message. Use {nameof(PublishSafe)} instead!");
+}
+else
+{
+ throw new InvalidOperationException($"Cannot subscribe to {type.Name} message while publishing. Use {nameof(PublishSafe)} instead!");
+}Also applies to: 148-149
There was a problem hiding this comment.
Files selected (1)
- Runtime/MessageBrokerService.cs (5)
Review comments generated (0)
- Review: 0
- LGTM: 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
New:
Changed:
Fixed:
Summary by CodeRabbit
Release Notes for Version 0.14.0
New Features
PublishSafe, for safer message publishing in chain subscriptions.Bug Fixes
CoroutineTests.Documentation
Tests
MessageBrokerService, including new tests for thePublishSafemethod and subscription behaviors.Chores
Summary by CodeRabbit
PublishSafemethod to theIMessageBrokerServiceinterface, allowing for safe message publishing.CoroutineTestsin theMessageBrokerServiceTest.csfile when building released projects.CHANGELOG.mdfile to reflect the changes made to theIMessageBrokerServiceinterface and the behavior of theSubscribeandUnsubscribemethods.SubscribeandUnsubscribemethods in theMessageBrokerService.csfile, improving their functionality.MessageBrokerServiceTest.csfile.InstallerTest.csfile and added a newusingdirective.