Skip to content
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

Prevent FindNextOpenClientSlot from returning duplicate IDs if called… #112

Closed

Conversation

tylerjwatson
Copy link
Member

… simultaneously in a multi-thread scenario.

This places a more granular locks in the FindNextOpenClientSlot method, effectively preventing it from returning duplicate IDs if ever called in parallel. It's the same fix as #108 but not locking against network I/O which is a good candidate for deadlocks.

I am going to need testing.

Tyler Watson added 2 commits November 4, 2016 11:20
@hakusaro
Copy link
Member

As a result of the mintaka changes, this has been obsoleted. @DeathCradle do we need to work on implementing this as a modification?

@SignatureBeef
Copy link
Member

@nicatronTg There are two ways of doing this:

  1. Use the existing Hooks.Net.Socket.Accepted OTAPI hook to reimplement Netplay.OnConnectionAccepted and use additional changes proposed here.
  2. A new TShock.Modification to reimplement Netplay.FindNextOpenClientSlot with these changes

Pro/con's for 1:

  • Pro. Should only require ~10 extra lines of code
  • Pro. No need to work with the OTAPI patcher - so other devs can fix or implement this
  • Con. The extra lines of code will need to be kept in sync with Terraria's changes

Pro/con's for 2:

  • Pro. Shouldn't change often/need to be kept in sync with Terraria's changes
  • Con. A new project, so more changes overall
  • Con. Less people know how modifications work

Either way is simple for me, let me know which you prefer.

@hakusaro
Copy link
Member

Well 2 pros on 1 and 1 on 2 means that 1 wins. Go with 1 c:

@SignatureBeef
Copy link
Member

Closing off as @tylerjwatson's changes have already been ported into Mintaka some time ago now.
See https://github.com/NyxStudios/TerrariaAPI-Server/blob/general-devel/TerrariaServerAPI/TerrariaApi.Server/Hooking/NetHooks.cs#L140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants