Data race in response payload for parallel requests

[shadow-cs]

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 :wink:.

[chkr1011]

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]

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

[chkr1011]

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

[shadow-cs]

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]

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 (https://github.com/shadow-cs/CoAPnet/commit/21e5fcec31ff623f700b85260b4e5115bfc3ae11) 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]

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]

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]

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]

New nuget version is released.