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

No responses come through after lots of activity #52

Open
Nevetos opened this issue May 9, 2020 · 4 comments
Open

No responses come through after lots of activity #52

Nevetos opened this issue May 9, 2020 · 4 comments

Comments

@Nevetos
Copy link

Nevetos commented May 9, 2020

Hello nikeee,

I have been using your API wrapper for a while (and made a few additions to it), and its been working like a charm. I just noticed that you started updating it again and I got really excited!

One thing that I have come across is when multiple requests occur around the same time, it will just error out with a null response.

e.g. multiple WhoAmI() requests, or GetClientInfo() requests at once.
I have gotten around this by just sending every request through a queue, so its not a major issue.

Is this something you would be willing/able to fix? As it would make things a little faster and smoother.

@nikeee
Copy link
Owner

nikeee commented May 9, 2020

The library uses a queue internally.

Maybe there is a race condition somewhere. I'll see if I can reproduce this.

@nikeee
Copy link
Owner

nikeee commented May 15, 2020

I just noticed the QueryClient uses a Queue<T> internally, which is not thread safe.
Replacing it with a ConcurrentQueue<T> may solve a problem (not sure if it would solve yours). But I think it would be better to rewrite the request/response handling entirely.

This seems to originate by the fact that this library is a port of the Node.js library. As Node.js is single-threaded, this was simply overlooked.

@cub1con
Copy link

cub1con commented Sep 7, 2022

Hi everyone, I think I know what the issue is.

Queue<T> isn't actually the problem.

If Send() is executed in quick succession or two instances at the same time, it is possible that _currentCommand is overwritten before the response is processed in ResponseProcessingLoop() and thus the task will not finish.

You are able to prove it by having multiple threads repeatedly running Send() in quick succession.

I fixed it on my fork by

  • having a task with a periodic timer running CheckQueue()
  • adding a check in CheckQueue() if _currentCommand != null to return
  • adding in Send() d.Task.ContinueWith((o) => { this._currentCommand = null; }); to reset _currentCommand after finishing its task

Maybe not the best solution, but it works without any lockups now. https://github.com/cub1con/TeamSpeak3QueryApi/blob/master/src/TeamSpeak3QueryApi/QueryClient.cs

@nikeee
Copy link
Owner

nikeee commented Sep 7, 2022

Thanks for pointing this out, I'll have a look at this when I've got some spare time.

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

No branches or pull requests

3 participants