FirstGearGames
Avoid collection modified exception
This adds an "iterable list" which supports removing from the list mid iteration
https://github.com/FirstGearGames/FishySteamworks/issues/9
I'm in mobile so let me know if I reviewed this wrong.
I'm not sure about the for loop to remove. I know it probably won't happen all that often but before we commit to this approach what if we did the same thing but with a dictionary?
When an add, modify, or remove occurs during an iteration it's instead cached and removed after the iteration.
On Wed, Jul 24, 2024, 8:24 PM Peter Giuntoli @.***> wrote:
This adds an "iterable list" which supports removing from the list mid iteration
#9 https://github.com/FirstGearGames/FishySteamworks/issues/9
You can view, comment on, or merge this pull request online at:
https://github.com/FirstGearGames/FishySteamworks/pull/13 Commit Summary
- 5b3d1cf https://github.com/FirstGearGames/FishySteamworks/pull/13/commits/5b3d1cf80fbb8512cd8d7daea29c6cbb0869988a Avoid collection modified exception
File Changes
(2 files https://github.com/FirstGearGames/FishySteamworks/pull/13/files )
- A FishNet/Plugins/FishySteamworks/Core/IterableList.cs https://github.com/FirstGearGames/FishySteamworks/pull/13/files#diff-c8d0ff2a15f9fd88e30351622a997eec39a2d20285257d303c3b78cfc35a5089 (83)
- M FishNet/Plugins/FishySteamworks/Core/ServerSocket.cs https://github.com/FirstGearGames/FishySteamworks/pull/13/files#diff-794714384a98a6c68abd11af3cb29560daeb1db70f05ca5eb673d58b056ebf37 (28)
Patch Links:
- https://github.com/FirstGearGames/FishySteamworks/pull/13.patch
- https://github.com/FirstGearGames/FishySteamworks/pull/13.diff
— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishySteamworks/pull/13, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3X3DOJ35ZN2ZZPPFU3ZOBAVPAVCNFSM6AAAAABLNPJXZCVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQZDQNRZGUYTKMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
That sounds fine to me. We have a similar concept where we mark collections as "dirty" with pending operations that are performed once iteration is complete.
I'm out for the rest of the day but could make that change tomorrow.
This functionality could be made into its own class but to save time I may just code the logic into the socket scripts.
On Wed, Jul 24, 2024, 9:00 PM Peter Giuntoli @.***> wrote:
That sounds fine to me. We have a similar concept where we mark collections as "dirty" with pending operations that are performed once iteration is complete.
— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishySteamworks/pull/13#issuecomment-2249144396, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3QCRZVAU5PMXKMD3ODZOBE4PAVCNFSM6AAAAABLNPJXZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBZGE2DIMZZGY . You are receiving this because you commented.Message ID: @.***>
I haven't tested the updates thoroughly yet but I think they should match what you're looking for.
Made some changes and tested a disconnect mid iteration this morning and this seems to do the trick.
@FirstGearGames It's been a few weeks, what do you think about this change?
Sorry been exceptionally busy. I took a better look and my only concern is that with your change the FishNet events would invoke for connect/disconnect before the collection is actually modified.
So if a dev were to try and send data during the invoke things might go south. I have not tried this scenario yet but it seems like a possibility.
You're welcome to give said scenario a shot and let me know if it passes for connect/disconnect, if so we can go ahead with your PR.
If the test fails, please give this package a try. FishySteamworks_Core.zip
For context, we've been running with this code for around a month now with no perceived issues. I tried to test what you were asking but I'm not sure I fully understand the scenario you're describing.
I ended up adding this code which a remove client requests a server to "kick them" which triggers an immediate disconnect and then we send a broadcast message back.
private void OnKickMe (NetworkConnection conn, CheatKickMe _, Channel channel) {
conn.Disconnect(immediately: true);
conn.Broadcast(new SomeMessage());
}
And logged the following which is what I would expect since this doesn't seem like a "valid" scenario. As expected the connection is gone and the message fails to send.
Edit
Changing up to a Disconnect(immediately: false) has a different set of logs but the same expected behavior, a single error about the connection not being valid and being unable to send the broadcast.
Try to send a message through in the OnRemoteConnectionState callback and ensure they get it.
On Thu, Aug 15, 2024, 4:15 PM Peter Giuntoli @.***> wrote:
For context, we've been running with this code for around a month now with no perceived issues. I tried to test what you were asking but I'm not sure I fully understand the scenario you're describing.
I ended up adding this code which a remove client requests a server to "kick them" which triggers an immediate disconnect and then we send a broadcast message back.
private void OnKickMe (NetworkConnection conn, CheatKickMe _, Channel channel) { conn.Disconnect(immediately: true); conn.Broadcast(new SomeMessage()); }
And logged the following which is what I would expect since this doesn't seem like a "valid" scenario. As expected the connection is gone and the message fails to send.
log.txt https://github.com/user-attachments/files/16629877/log.txt
— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishySteamworks/pull/13#issuecomment-2292147264, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3TACCXS7P3ZIQP5A3DZRUD4TAVCNFSM6AAAAABLNPJXZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJSGE2DOMRWGQ . You are receiving this because you were mentioned.Message ID: @.***>
I sent a broadcast through that and hit an authentication error. Without requiring authentication the remote client doesn't get it but that's the same behavior without this change so not sure if that's a bug with this PR or something existing. Don't have time to track it down and unfortunately I'm headed out to Gamescom so won't be able to poke around for a week or so.
EDIT
I just tested with your .zip and it looks like the same behavior.
Fairly sure it's a bug with the PR assuming it's something about the client not found. The files I provided should work.
On Thu, Aug 15, 2024, 7:44 PM Peter Giuntoli @.***> wrote:
I sent a broadcast through that and hit an authentication error. Without requiring authentication the remote client doesn't get it but that's the same behavior without this change so not sure if that's a bug with this PR or something existing. Don't have time to track it down and unfortunately I'm headed out to Gamescom so won't be able to poke around for a week or so.
— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishySteamworks/pull/13#issuecomment-2292464579, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3WCDHBPCGIYUER5ZVLZRU4MRAVCNFSM6AAAAABLNPJXZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJSGQ3DINJXHE . You are receiving this because you were mentioned.Message ID: @.***>
Fairly sure it's a bug with the PR assuming it's something about the client
not found. The files I provided should work.
They had the same behavior.
Corrected one other possible issue with my changes. I recall you said you're going out but this SHOULD resolve all issuees. FishySteamworks_Full.zip
Going to merge this but replace it with my implementation. I believe your approach would be a race condition in the scenario I mentioned.
Going to merge this but replace it with my implementation. I believe your approach would be a race condition in the scenario I mentioned.
Just got back from Germany and poking at the changes that ended up getting pushed. Seems a little harder to maintain the separate bool and list since now it's on the user to remember they need to do that. I also don't think there's any functional difference with what you've pushed since the code just exists outside the bidirectional dictionary instead of inside. There's also no support for iterating the list twice if that ends up happening in the future.
Thanks for pushing a fix though!
There's no extra work for the user. The list and boolean are handled internally in the transport.
The main difference, and why I went this route, is because with your implementation the events were being invoked before the collection was modified, which could result in entries missing if the user were utilizing those events.
The alternative, being the one I put in, the events dispatch only after the collection is modified. It caches the changes similar to yours but doesn't invoke until the cached changes can be iterated.
Not sure I'm following for what you mean by iterating the list twice?
On Tue, Aug 27, 2024, 12:58 PM Peter Giuntoli @.***> wrote:
Going to merge this but replace it with my implementation. I believe your approach would be a race condition in the scenario I mentioned.
Just got back from Germany and poking at the changes that ended up getting pushed. Seems a little harder to maintain the separate bool and list since now it's on the user to remember they need to do that. I also don't think there's any functional difference with what you've pushed since the code just exists outside the bidirectional dictionary instead of inside. There's also no support for iterating the list twice if that ends up happening in the future.
Thanks for pushing a fix though!
— Reply to this email directly, view it on GitHub https://github.com/FirstGearGames/FishySteamworks/pull/13#issuecomment-2313080317, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPJC3SSGN5KMOKMUAO4P7LZTSV3FAVCNFSM6AAAAABLNPJXZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJTGA4DAMZRG4 . You are receiving this because you modified the open/close state.Message ID: @.***>
There's no extra work for the user. The list and boolean are handled internally in the transport.
I meant for anyone doing work in the transport which might just be future you but could also be anyone you bring on the team or that tries to contribute to the repo. If the list is iterated in another spot, folks need to remember to copy the boolean setting/unset and flush of pending data.
The main difference, and why I went this route, is because with your implementation the events were being invoked before the collection was modified, which could result in entries missing if the user were utilizing those events.
I must be blind but I see everywhere that iterates _steamConnections calling ProcessPendingConnectionChanges() once the iteration is over. Where is the code that's invoking the events before the collection was modified? My code hid a similar call to ProcessPendingConnectionChanges() inside the enumerator itself but the result would be the same.
Maybe you're saying that this being above the loop of while (_clientHostIncoming.Count > 0) instead of above the foreach is the benefit? https://github.com/FirstGearGames/FishySteamworks/commit/21e858249249e2c322365fe9fefbe865f290b0d9#diff-794714384a98a6c68abd11af3cb29560daeb1db70f05ca5eb673d58b056ebf37R384
Not sure I'm following for what you mean by iterating the list twice?
If there's a recursive iteration there might be a bug because the iteration tracking is just a bool. Another "not an issue right now, but maybe an issue for future you" type thing.