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

Data race in response payload for parallel requests #11

Closed
shadow-cs opened this issue Mar 13, 2022 · 9 comments · Fixed by #12
Closed

Data race in response payload for parallel requests #11

shadow-cs opened this issue Mar 13, 2022 · 9 comments · Fixed by #12

Comments

@shadow-cs
Copy link

Hi, I was playing around with Tradfri (thanks a lot for this library BTW). And when I started making parallel requests I started noticing that there are weird situations where the message is trimmed (but maybe I'm even getting response to another request, it is difficult to tell in async context and given that all the responses are the same - device info to be exact).

I was able to fix this by augmenting CoapMessage with byte[] PayloadBytes and using that in CoapMessageToResponseConverter. I assign PayloadBytes here by just doing message.PayloadBytes = message.Payload.ToArray(); and voila the problem is gone (quick and dirty, but it did the job).

I didn't do more digging, but it makes me wonder whether the array segments aren't reused somewhere and reassigned before the converter has chance to pick up the data for the response.

This problem also occurs the most during jitting in steady state it is much less likely to occur.

I'll be happy to provide you with more feedback or a sample provided tou have a gateway to test it on 😉.

@chkr1011
Copy link
Owner

I created a branch for this issue. It adds a semaphore so that the access to the transport layer while sending is limited to one parallel thread. Please checkout this version and let me know if it fixes the issue. If not there might be more changes.

@shadow-cs
Copy link
Author

@chkr1011 thanks, I will just hang on please, as this is a hobby project I might not have enough time until the weekend 😉

@chkr1011
Copy link
Owner

chkr1011 commented Apr 5, 2022

Please let me know if the changes in branch 11-data-race-in-response-payload-for-parallel-requests fixing the issue.

@shadow-cs
Copy link
Author

Sorry to keep you waiting, I still have this in my personal backlog (didn't forget about it, just not enough time), I hope I'll manage to test it this weekend.

@shadow-cs
Copy link
Author

shadow-cs commented Apr 23, 2022

Hi, @chkr1011 nope unfortunately it didn't fix the issue. Collisions still occur. Also, I needed to change the initialCount of the semaphore to 1 😉.

I have a Sentry tracing in place and it clearly shows the requests being dispatched simultaneously.

Please take a closer look at my solution (shadow-cs@21e5fce) it prevents the race by copying the data ahead of time. A little more refactoring would be needed to possibly change Payload to byte[] (which is a breaking change, unfortunately) or just introduce a new property and set Payload from this one (so we avoid BC).

If your goal is to minimize allocations I suggest you use an array pool (on supported platforms), but CoapMessage would need to become IDisposable. But the ArraySegment.ToArray also allocates so this change only places the allocation in a different place.

PS.: Sorry it took so long.

@chkr1011
Copy link
Owner

I made again some changes in memory management. Basically I copied the memory as you already proposed. This should finally fix the issue 👍

@shadow-cs
Copy link
Author

I can confirm the threading issue is fixed 👍. Thanks.

From my performance monitoring, I do see a slight performance degradation (about 0.75ms for 18 requests I'm making). This isn't a concern for me, just pointing this out 😉. I didn't do any serious measurement and statistical analysis around it, just repeated a few dozen times and looked at the fastest operation.

@chkr1011
Copy link
Owner

The performance decrease is OK in my opinion because the payload gets copied now. I will release a new version including this fix at the coming weekend.

Thanks for reporting and testing.

@chkr1011 chkr1011 linked a pull request Apr 30, 2022 that will close this issue
chkr1011 added a commit that referenced this issue Apr 30, 2022
…-for-parallel-requests

Fix data race in response payload for parallel requests (#11)
@chkr1011
Copy link
Owner

New nuget version is released.

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 a pull request may close this issue.

2 participants