This set of changes has the primary goal of addressing concurrency issues and corner cases related to state management within the GameStateManager with respect to the use of static fields and concurrency protections during blob storage updates.
In addition, due to the large amount of churn, some reformatting and refactoring changes have been included. Though they're largely non-critical and focused on the primary goal, they do address some build and linter warnings to bring the repository and source structures to current best practices, and allow for consistency across the code.
While these changes do add some functionality, my intent was for to only address those areas which make the core game experience more reliable, consistent, and solid without taking away the spots that were originally left for potential enhancements at the hack. I purposefully introduced some new areas of enhancement to replace those that I addressed, some easy and some harder. I'll be documenting these more explicitly in a near future set of changes. I hope that doesn't throw things off too badly.
Implementation Specifics
Concurrency
Converted the GameStateManager to a fully asynchronous implementation, removing the calls to block on Task-based operations to make things synchronous. These practices can lead to deadlocks and unobserved exceptions, especially in scenarios with concurrent callers such as this.
Removed the static state variable within the GameStateManager. It was not protected by any concurrency guards and could end up in an inconsistent state should there be competing function invocations.
Ensured that all state operations (both reads and writes) happen atomically within a blob lease, to prevent any inconsistencies between function instances (running in different hosts). Previously, the function instance assumed that it was the only active instance and the only consumer of the state from storage. The state blob was read once into a static variable and never refreshed from storage. Changes were pushed to storage with no concurrency guard. If there were multiple instances of the functions running, they were unable to synchronize and would be competing over state ownership, potentially corrupting state and trampling each other. Even with the low scale, additional instances could be deployed into the environment by the host infrastructure for machine updates, or just because.
Ensured that all methods marked async are correctly returning a Task. Previously, there were some instances of async void methods, which potentially leak exceptions and otherwise hamper the framework.
Ensured that all await operations in non-function code denote that they do not need the context restored; calls within the function implementation will continue to restore the appropriate context for working within the web framework.
Added extensions to allow a task whose results were not desired to be run in a fire-and-forget way, while still capturing any exceptions that may occur.
Functionality
Ensured that all constructs which are disposable are properly closed and disposed in their scope; previously, some of the streams used were neither.
Added a common path for storage-related activities, ensuring consistency of lease acquisition/release and some lightweight, general purpose, retries provided by the storage SDK.
Added an enhanced set of state metadata, both to allow callers to get a more accurate and full picture of a game in progress as well as to move all state into the structure so that it can migrate between function instances as needed via storage.
Enhanced the flow for ping generation and management, making the API ultimately responsible for detecting a missed pong and ejecting the device from the game. This happens as part of some critical game-related actions, such as StartGame and Pong with a scheduled event running to ensure that the game doesn't stall due to communication failures or other issues.
Added communication methods for additional events on the button devices; these events don't currently exist but, instead, the framework is provided as a potential low-hanging item for enhancement during the hack.
Structure, Documentation, and Miscellaneous
Moved responsibility for reading configuration to the function level only, isolating the other game components from having knowledge of their environment. Configuration values are now injected via constructors into the GameStateManager and elsewhere.
Separated out utility classes and transfer objects into their own files unless they cannot stand on their own context, as is C# standard convention. Made classes nested when they only make sense in the context of another.
Updated conditional error traps to use exception filters, removing logic from the blocks to make for a smaller surface area for exception handling and to conform to best practices.
Refactored API endpoints to make use of the framework for bindings rather than directly reading content bodies and performing the serialization/binding tasks.
Refactored the API endpoints to make use of the IActionResult constructs instead of manually manipulating the raw HttpResponseMessage for standard cases.
Refactored API endpoints to take a neutral logging interface instead of the TraceLogger to increase testability, should someone ever get to adding unit tests.
Refactored HTTP communication to make use of the modern HttpClient and remove the old WebRequest constructs. This also allows for more streamlined and simplified creation of the form-encoded payloads.
Removed and sorted using directives and other small formatting tweaks as recommended by static analysis.
Refactored licensing headers into a common licence file in the root of the repository, as is the currently accepted practice. This removes the need to redundantly update every source file should the license or its terms change, allowing easier updates over time. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it just felt wrong to paste into all the new classes that I was adding)
Removed the change log from source files, as the source control system is the authority on changes and the manually updated log is typically considered redundant and a source of inaccuracies. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it just felt wrong to paste into all the new classes that I was adding)
Refactored the namespace to confirm to the C# standard of disallowing consecutive capital letters. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it was setting off my OCD)
Added XML comment headers to all constructs, because I'm a crazed OCD nutcase.
Pending Changes
Further testing; I believe the core scenarios are working well, but I wanted to get these out as I was able, given that I'm later than I wanted to be and the changes are more extensive.
More documentation changes; I want to update some ReadMe areas as well as do an additional ReadMe on the C# API calling out some of the areas for potential enhancement that I left.
Overview
This set of changes has the primary goal of addressing concurrency issues and corner cases related to state management within the
GameStateManagerwith respect to the use ofstaticfields and concurrency protections during blob storage updates.In addition, due to the large amount of churn, some reformatting and refactoring changes have been included. Though they're largely non-critical and focused on the primary goal, they do address some build and linter warnings to bring the repository and source structures to current best practices, and allow for consistency across the code.
While these changes do add some functionality, my intent was for to only address those areas which make the core game experience more reliable, consistent, and solid without taking away the spots that were originally left for potential enhancements at the hack. I purposefully introduced some new areas of enhancement to replace those that I addressed, some easy and some harder. I'll be documenting these more explicitly in a near future set of changes. I hope that doesn't throw things off too badly.
Implementation Specifics
Concurrency
Converted the
GameStateManagerto a fully asynchronous implementation, removing the calls to block onTask-based operations to make things synchronous. These practices can lead to deadlocks and unobserved exceptions, especially in scenarios with concurrent callers such as this.Removed the static state variable within the
GameStateManager. It was not protected by any concurrency guards and could end up in an inconsistent state should there be competing function invocations.Ensured that all state operations (both reads and writes) happen atomically within a blob lease, to prevent any inconsistencies between function instances (running in different hosts). Previously, the function instance assumed that it was the only active instance and the only consumer of the state from storage. The state blob was read once into a static variable and never refreshed from storage. Changes were pushed to storage with no concurrency guard. If there were multiple instances of the functions running, they were unable to synchronize and would be competing over state ownership, potentially corrupting state and trampling each other. Even with the low scale, additional instances could be deployed into the environment by the host infrastructure for machine updates, or just because.
Ensured that all methods marked
asyncare correctly returning aTask. Previously, there were some instances ofasync voidmethods, which potentially leak exceptions and otherwise hamper the framework.Ensured that all
awaitoperations in non-function code denote that they do not need the context restored; calls within the function implementation will continue to restore the appropriate context for working within the web framework.Added extensions to allow a task whose results were not desired to be run in a fire-and-forget way, while still capturing any exceptions that may occur.
Functionality
Ensured that all constructs which are disposable are properly closed and disposed in their scope; previously, some of the streams used were neither.
Added a common path for storage-related activities, ensuring consistency of lease acquisition/release and some lightweight, general purpose, retries provided by the storage SDK.
Added an enhanced set of state metadata, both to allow callers to get a more accurate and full picture of a game in progress as well as to move all state into the structure so that it can migrate between function instances as needed via storage.
Enhanced the flow for ping generation and management, making the API ultimately responsible for detecting a missed
pongand ejecting the device from the game. This happens as part of some critical game-related actions, such asStartGameandPongwith a scheduled event running to ensure that the game doesn't stall due to communication failures or other issues.Added communication methods for additional events on the button devices; these events don't currently exist but, instead, the framework is provided as a potential low-hanging item for enhancement during the hack.
Structure, Documentation, and Miscellaneous
Moved responsibility for reading configuration to the function level only, isolating the other game components from having knowledge of their environment. Configuration values are now injected via constructors into the
GameStateManagerand elsewhere.Separated out utility classes and transfer objects into their own files unless they cannot stand on their own context, as is C# standard convention. Made classes nested when they only make sense in the context of another.
Updated conditional error traps to use exception filters, removing logic from the blocks to make for a smaller surface area for exception handling and to conform to best practices.
Refactored API endpoints to make use of the framework for bindings rather than directly reading content bodies and performing the serialization/binding tasks.
Refactored the API endpoints to make use of the
IActionResultconstructs instead of manually manipulating the rawHttpResponseMessagefor standard cases.Refactored API endpoints to take a neutral logging interface instead of the
TraceLoggerto increase testability, should someone ever get to adding unit tests.Refactored HTTP communication to make use of the modern
HttpClientand remove the oldWebRequestconstructs. This also allows for more streamlined and simplified creation of the form-encoded payloads.Removed and sorted
usingdirectives and other small formatting tweaks as recommended by static analysis.Refactored licensing headers into a common licence file in the root of the repository, as is the currently accepted practice. This removes the need to redundantly update every source file should the license or its terms change, allowing easier updates over time. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it just felt wrong to paste into all the new classes that I was adding)
Removed the change log from source files, as the source control system is the authority on changes and the manually updated log is typically considered redundant and a source of inaccuracies. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it just felt wrong to paste into all the new classes that I was adding)
Refactored the namespace to confirm to the C# standard of disallowing consecutive capital letters. (sorry, I feel badly about this as it feels more intrusive than I intended to be, but it was setting off my OCD)
Added XML comment headers to all constructs, because I'm a crazed OCD nutcase.
Pending Changes
Further testing; I believe the core scenarios are working well, but I wanted to get these out as I was able, given that I'm later than I wanted to be and the changes are more extensive.
More documentation changes; I want to update some ReadMe areas as well as do an additional ReadMe on the C# API calling out some of the areas for potential enhancement that I left.
Addressed Issues
10