Implement AsyncEvents #1
Conversation
|
WalkthroughThe changes in this pull request involve a significant transition to asynchronous event handling across multiple classes in the Changes
Sequence Diagram(s)sequenceDiagram
participant Game
participant Monitor
participant Cpu
Monitor->>Game: Subscribe to OnStartedAsync
Monitor->>Game: Subscribe to OnQuitAsync
Game->>Monitor: OnStartedAsync
Monitor->>Cpu: Handle CPU step
Game->>Monitor: OnQuitAsync
Monitor->>Cpu: Cleanup on quit
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sonarcsharp (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range comments (2)
ppsspp-api.csproj (2)
Line range hint
14-15: Ace! Including symbols for better debugging.Setting IncludeSymbols to True and specifying the SymbolPackageFormat as 'snupkg' are top-notch changes. These will make debugging a breeze for developers using your package.
Just a small suggestion: Consider adding a comment explaining the benefits of including symbols and using the 'snupkg' format for future maintainers.
Line range hint
16-16: Fair dinkum! Enforcing code style during build.Setting EnforceCodeStyleInBuild to True is a beauty. This will help maintain consistent code quality across the project by enforcing code style rules during the build process.
To complement this, you might want to consider adding an .editorconfig file to the project if you haven't already. This would ensure that all developers are using the same code style settings in their IDEs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- Endpoints/Cpu.cs (3 hunks)
- Endpoints/Game.cs (4 hunks)
- Endpoints/Input.cs (1 hunks)
- MonitorFileOpen/Program.cs (2 hunks)
- Ppsspp.cs (8 hunks)
- ResultMessage.cs (1 hunks)
- ppsspp-api.csproj (1 hunks)
🔇 Additional comments (11)
ppsspp-api.csproj (3)
20-20: G'day! Let's verify if this version bump is appropriate.The version has been updated from 0.0.2-pre to 0.0.3-pre. This minor version increment suggests notable changes or improvements in the project. Could you please confirm that this version bump aligns with the scope of changes in this PR, particularly the implementation of AsyncEvents?
Line range hint
9-9: Bonza! Enabling XML documentation generation.Setting GenerateDocumentationFile to True is a ripper move. This will generate XML documentation files during the build, which can enhance IntelliSense support and make it easier for developers to understand and use your API. Good on ya for improving the project's documentation!
Line range hint
1-85: Strewth! Your project setup is looking schmick.I've noticed a few more things that are worth a mention:
You're using static code analyzers like Microsoft.VisualStudio.Threading.Analyzers and SonarAnalyzer.CSharp. That's a ripper idea for maintaining high code quality.
The file exclusions for directories like HelloWorld, MonitorFileOpen, and Memory seem intentional. If these are test or example projects, you might consider moving them to a separate solution folder to keep your main project clean.
The use of PackageReference for managing dependencies is spot on. It's the recommended approach for modern .NET projects.
Overall, your project structure and setup show good practices. Keep up the good work!
Endpoints/Cpu.cs (2)
1-2: Import statements are appropriate.The necessary namespaces have been included.
8-9: Use ofAsyncEventenhances asynchronous handling.The transition to
AsyncEventforOnStepandOnResumesupports improved asynchronous event management.MonitorFileOpen/Program.cs (3)
42-46: Looks good!The
Game_OnQuitAsyncmethod correctly removes the breakpoint when the game quits, ensuring that resources are properly cleaned up.
71-71: Good practice changing 'async void' to 'async Task'Updating the
Cpu_OnStepmethod to returnTaskinstead ofvoidenhances exception handling and allows for better control over asynchronous operations.
Line range hint
71-95: Looks good!The
Cpu_OnStepmethod effectively handles CPU stepping logic with appropriate null checks and exception handling.Endpoints/Game.cs (2)
18-18: EnsureAsyncEventusage addresses race conditions appropriately.Given the PR's objective to resolve race conditions related to event processing, confirm that
AsyncEventis suitable for this purpose and that its usage addresses the concurrency issues previously experienced. Ensure that all event invocations are correctly awaited and that subscribers handle events asynchronously as intended.Also applies to: 27-27, 40-40, 53-53
18-18: Update event subscribers to handleAsyncEvent.The events
OnStarted,OnQuit,OnPaused, andOnResumedhave been changed from traditional event handlers toAsyncEvent. Ensure that all existing event subscribers have been updated to handleAsyncEventappropriately to prevent runtime issues.Run the following script to identify any usages of the old event patterns:
Also applies to: 27-27, 40-40, 53-53
Ppsspp.cs (1)
202-205:⚠️ Potential issueVerify the asynchronous nature of 'SendInstant'
The method
_socket.SendInstant(...)is being awaited, but if it's synchronous, usingawaitis unnecessary and may cause confusion. Verify whetherSendInstantis asynchronous. If it's synchronous, remove theawaitkeyword.Run the following script to verify if
SendInstantis asynchronous:
| <IncludeSymbols>True</IncludeSymbols> | ||
| <SymbolPackageFormat>snupkg</SymbolPackageFormat> | ||
| <EnforceCodeStyleInBuild>True</EnforceCodeStyleInBuild> | ||
| <LangVersion>default</LangVersion> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heads up on the LangVersion setting, mate.
The LangVersion is currently set to 'default', which is fair enough as it allows the use of the latest C# features compatible with your target framework. However, for better control and clarity, you might want to consider explicitly specifying the C# version you're targeting. For example, if you're using C# 11 features, you could set it to:
<LangVersion>11.0</LangVersion>This can help prevent unexpected behaviour if the project is built on a system with a different default C# version.
| public AsyncEvent<InputButtonResult> OnButtonChange = new(); | ||
| public AsyncEvent<InputAnalogResult> OnAnalogChange = new(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate AsyncEvent instances to protect event integrity
Currently, OnButtonChange and OnAnalogChange are declared as public fields. This allows external code to modify or replace the event instances, which could lead to unintended behaviour. To maintain encapsulation and safeguard your event handling mechanism, consider making these fields private and exposing them via public read-only properties.
You might refactor the code as follows:
-public AsyncEvent<InputButtonResult> OnButtonChange = new();
-public AsyncEvent<InputAnalogResult> OnAnalogChange = new();
+private readonly AsyncEvent<InputButtonResult> _onButtonChange = new();
+public AsyncEvent<InputButtonResult> OnButtonChange => _onButtonChange;
+
+private readonly AsyncEvent<InputAnalogResult> _onAnalogChange = new();
+public AsyncEvent<InputAnalogResult> OnAnalogChange => _onAnalogChange;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public AsyncEvent<InputButtonResult> OnButtonChange = new(); | |
| public AsyncEvent<InputAnalogResult> OnAnalogChange = new(); | |
| private readonly AsyncEvent<InputButtonResult> _onButtonChange = new(); | |
| public AsyncEvent<InputButtonResult> OnButtonChange => _onButtonChange; | |
| private readonly AsyncEvent<InputAnalogResult> _onAnalogChange = new(); | |
| public AsyncEvent<InputAnalogResult> OnAnalogChange => _onAnalogChange; |
|
|
||
| public async ValueTask<CpuGetReg> GetRegisterAsync(string register) | ||
| { | ||
| ArgumentException.ThrowIfNullOrEmpty(register); |
There was a problem hiding this comment.
Replace ArgumentException with ArgumentNullException in ThrowIfNullOrEmpty.
The method ThrowIfNullOrEmpty is a static method of ArgumentNullException, not ArgumentException. Using ArgumentException will result in a compilation error.
Apply this diff to correct the exception type:
- ArgumentException.ThrowIfNullOrEmpty(register);
+ ArgumentNullException.ThrowIfNullOrEmpty(register);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ArgumentException.ThrowIfNullOrEmpty(register); | |
| ArgumentNullException.ThrowIfNullOrEmpty(register); |
| throw new Exception("No functions, not playing any game?"); | ||
| } | ||
|
|
||
| functionAddress = result2.Functions.SingleOrDefault(x => x.Name == "zz_sceIoOpen")?.Address; |
There was a problem hiding this comment.
Function name inconsistency: 'zz_sceIoOpen' vs 'sceIoOpen'
There's a mismatch between the function name used in the search and the exception message. You're searching for "zz_sceIoOpen" but the exception refers to "sceIoOpen". Please confirm the correct function name and ensure consistency.
If "sceIoOpen" is the correct function name, apply this diff:
-functionAddress = result2.Functions.SingleOrDefault(x => x.Name == "zz_sceIoOpen")?.Address;
+functionAddress = result2.Functions.SingleOrDefault(x => x.Name == "sceIoOpen")?.Address;Alternatively, if "zz_sceIoOpen" is correct, update the exception message:
-throw new Exception("Function stub for sceIoOpen not found... game never calls it?");
+throw new Exception("Function stub for zz_sceIoOpen not found... game never calls it?");Also applies to: 59-62
| //await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:45333/debugger")); | ||
| //await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:52809/debugger")); | ||
| //await ppsspp.ConnectAsync(new Uri("ws://10.211.55.3:52809/debugger")); //WindowsVM | ||
| //await ppsspp.ConnectAsync(new Uri("ws://192.168.1.31:45333/debugger")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code for a cleaner codebase
The commented-out connection lines are redundant now that AutoConnectAsync() is in use. Removing them will improve code readability and maintainability.
Apply this diff to remove the unnecessary code:
-//await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:45333/debugger"));
-//await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:52809/debugger"));
-//await ppsspp.ConnectAsync(new Uri("ws://10.211.55.3:52809/debugger")); //WindowsVM
-//await ppsspp.ConnectAsync(new Uri("ws://192.168.1.31:45333/debugger"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:45333/debugger")); | |
| //await ppsspp.ConnectAsync(new Uri("ws://127.0.0.1:52809/debugger")); | |
| //await ppsspp.ConnectAsync(new Uri("ws://10.211.55.3:52809/debugger")); //WindowsVM | |
| //await ppsspp.ConnectAsync(new Uri("ws://192.168.1.31:45333/debugger")); |
| [JsonIgnore] | ||
| public JsonElement Data { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review accessibility of the Data property in MessageEventArgs
Since the Data property is marked with [JsonIgnore], consider making it protected or providing a private setter to prevent unintended external modification. This enhances encapsulation and prevents potential misuse.
Apply the following diff to adjust the property's accessibility:
public class MessageEventArgs : EventArgs
{
[JsonPropertyName("event")]
public string Event { get; set; }
[JsonPropertyName("message")]
public string Message { get; set; }
[JsonPropertyName("ticket")]
public string Ticket { get; set; }
[JsonIgnore]
- public JsonElement Data { get; set; }
+ public JsonElement Data { get; protected set; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [JsonIgnore] | |
| public JsonElement Data { get; set; } | |
| } | |
| [JsonIgnore] | |
| public JsonElement Data { get; protected set; } | |
| } |
| public void Dispose() | ||
| { | ||
| if (_socket != null && (_socket.IsStarted || _socket.IsRunning)) | ||
| { | ||
| _socket.StopOrFail(WebSocketCloseStatus.NormalClosure, "Disconnected from PPSSPP").RunSynchronously(); | ||
| } | ||
| FailAllPendingAsync("Disconnected from PPSSPP").RunSynchronously(); | ||
| if (!disposedValue) | ||
| { | ||
| // TODO: dispose managed state (managed objects) | ||
| _socket?.Dispose(); | ||
|
|
||
| // TODO: free unmanaged resources (unmanaged objects) and override finalizer | ||
| // TODO: set large fields to null | ||
| _socket = null; | ||
| disposedValue = true; | ||
| } | ||
| OnClose?.InvokeAsync(this, EventArgs.Empty).RunSynchronously(); | ||
| //GC.SuppressFinalize(this); | ||
| } |
There was a problem hiding this comment.
Avoid synchronously blocking in 'Dispose' method
In the Dispose method, asynchronous methods are being invoked synchronously using .RunSynchronously(), which can lead to deadlocks or exceptions. Since Dispose cannot be asynchronous, consider refactoring to ensure that resources are disposed of properly without blocking.
One possible solution is to perform all necessary asynchronous cleanup in DisposeAsync and avoid calling asynchronous methods in Dispose. Alternatively, you could use the DisposeAsync().AsTask().GetAwaiter().GetResult() pattern, but this still risks blocking the thread.
| ["ReceivedMessage"] = args.GetRawText(), | ||
| }, | ||
| }); | ||
| tcs.SetException(new Exception(args.GetProperty("message").GetString()) { Data = { ["ReceivedMessage"] = args.GetRawText() }, Source = sender.ToString()}); |
There was a problem hiding this comment.
Cannot set the 'Source' property of an Exception
The 'Source' property of an Exception is read-only and cannot be set using an object initialiser. Assigning to it will result in a runtime exception. Consider removing the assignment to 'Source' or use another method to include sender information.
Apply this diff to fix the issue:
-tcs.SetException(new Exception(args.GetProperty("message").GetString()) { Data = { ["ReceivedMessage"] = args.GetRawText() }, Source = sender.ToString()});
+tcs.SetException(new Exception(args.GetProperty("message").GetString())
+{
+ Data = { ["ReceivedMessage"] = args.GetRawText(), ["Sender"] = sender.ToString() }
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tcs.SetException(new Exception(args.GetProperty("message").GetString()) { Data = { ["ReceivedMessage"] = args.GetRawText() }, Source = sender.ToString()}); | |
| tcs.SetException(new Exception(args.GetProperty("message").GetString()) | |
| { | |
| Data = { ["ReceivedMessage"] = args.GetRawText(), ["Sender"] = sender.ToString() } | |
| }); |
| private static readonly ThreadLocal<Random> Random = new (() => new Random()); | ||
|
|
||
| private static string MakeTicket(IReadOnlyDictionary<string, AsyncEvent<JsonElement>> pendingTickets) | ||
| { | ||
| const string chars = "0123456789abcdefghijklmnopqrstuvwxyz"; | ||
| var random = new Random(); | ||
|
|
||
|
|
||
| while (true) | ||
| { | ||
| var ticket = new string(Enumerable.Repeat(chars, 11) | ||
| .Select(s => s[random.Next(s.Length)]) | ||
| .Select(s => s[Random.Value!.Next(s.Length)]) | ||
| .ToArray()); | ||
|
|
||
| if (_pendingTickets.ContainsKey(ticket)) | ||
| if (pendingTickets.ContainsKey(ticket)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use 'Guid.NewGuid()' for unique ticket generation
Using ThreadLocal<Random> and manual string generation may lead to collisions and isn't the most efficient method. Consider using Guid.NewGuid() to generate unique tickets reliably and thread-safely.
Apply this diff to refactor the ticket generation:
-private static readonly ThreadLocal<Random> Random = new (() => new Random());
...
- const string chars = "0123456789abcdefghijklmnopqrstuvwxyz";
...
- var ticket = new string(Enumerable.Repeat(chars, 11)
- .Select(s => s[Random.Value!.Next(s.Length)])
- .ToArray());
+ var ticket = Guid.NewGuid().ToString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static readonly ThreadLocal<Random> Random = new (() => new Random()); | |
| private static string MakeTicket(IReadOnlyDictionary<string, AsyncEvent<JsonElement>> pendingTickets) | |
| { | |
| const string chars = "0123456789abcdefghijklmnopqrstuvwxyz"; | |
| var random = new Random(); | |
| while (true) | |
| { | |
| var ticket = new string(Enumerable.Repeat(chars, 11) | |
| .Select(s => s[random.Next(s.Length)]) | |
| .Select(s => s[Random.Value!.Next(s.Length)]) | |
| .ToArray()); | |
| if (_pendingTickets.ContainsKey(ticket)) | |
| if (pendingTickets.ContainsKey(ticket)) | |
| private static string MakeTicket(IReadOnlyDictionary<string, AsyncEvent<JsonElement>> pendingTickets) | |
| { | |
| while (true) | |
| { | |
| var ticket = Guid.NewGuid().ToString(); | |
| if (pendingTickets.ContainsKey(ticket)) |
| OnClose?.InvokeAsync(this, EventArgs.Empty).Wait(); | ||
|
|
||
| FailAllPending($"PPSSPP disconnected {info.Exception?.Message}"); | ||
| FailAllPendingAsync($"PPSSPP disconnected {info.Exception?.Message}").Wait(); |
There was a problem hiding this comment.
Avoid blocking with '.Wait()' on asynchronous methods
Using .Wait() on asynchronous methods like InvokeAsync and FailAllPendingAsync can cause deadlocks or block the calling thread. It's better to use await to asynchronously wait for these methods to complete.
Apply this diff to fix the issue:
-websocketClient?.DisconnectionHappened.Subscribe(info =>
-{
- OnClose?.InvokeAsync(this, EventArgs.Empty).Wait();
- FailAllPendingAsync($"PPSSPP disconnected {info.Exception?.Message}").Wait();
-});
+websocketClient?.DisconnectionHappened.Subscribe(async info =>
+{
+ await OnClose?.InvokeAsync(this, EventArgs.Empty);
+ await FailAllPendingAsync($"PPSSPP disconnected {info.Exception?.Message}");
+});Committable suggestion was skipped due to low confidence.
This is an attempt to get to the bottom of a perceived race condition.
Duplicate messages are being raised as an event causing already expired tickets to be attempted to be reprocessed.
Locks during breakpoints are never released as a duplicate set breakpoints are coming through with no pending ticket, as it was parsed previously.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores