Thread Safe Build

[benjamingr]

Hello, I'm running CsQuery in Azure and I'm getting a lot of thread safety issues.

I looked into the code and noticed that the problem is in RangeSortedDictionary, I've also seen that this issue is already addressed in the code with a thread safety flag. I'd just like to note that I'm not performing any threading myself, this is all azure internals

Specifically I'm getting an index out of bounds at System.Collections.Generic.Dictionary`2.Resize() .
I'm not running two threads on the same CQ instance btw.

My question is: would it be possible to get a thread safe built version of CsQuery in NuGet?

[jamietre]

Compiling the code with the thread-safety flag turned on should indeed make the CQ object thread-safe, but at the cost of performance. That code was written because of a bug that turned out to have nothing to do with threading. I left it in, but didn't turn it on because it hurts performance. Did you try compiling it yourself with thread safety enabled? Does it solve the problem?

The history of that is from troubleshooting a bug that appeared to be a thread safety issue, in fact it was the exact same .NET dictionary exception, but turned out to be an equality comparison bug. It turned up and was pretty difficult to troubleshoot because it manifested differently, or not at all, depending on the .NET dictionary implementation across framework versions.

I don't know too much about Azure, but since you aren't writing multiple-reentrant code then I am concerned that this could be the same bug, still not fixed. If you haven't tried compiling with that thread-safety flag, could you try that? If it's the bug, then this should not solve the problem, and that would tell me I need to take another approach.

[benjamingr]

You are correct, this is not a thread safety issue indeed.

I've read the thread about the issue. The odd thing is that it works perfectly fine when I run it locally but I get the bug when I run it in Azure.

In my case it happened when executing a "Remove" command, for what I do using "Empty" is enough, using "Empty" instead of remove does not cause it to throw the exception.

It seems that the file causing that has a wrong doctype, that might also be related, I'll look into it and let you know if I find anything interesting.

[jamietre]

Btw what framework version is the app built upon? 4 or 4.5? Is there any possibility that it could be a different version on Azure vs. your dev environment? For example if you are using CsQuery in a class library built on 4.0, but loaded in a production environment built on 4.5, it could end up using different versions of the framework classes. Just trying to get as much info as I can to figure out if this is likely the same old bug before I dig into it.

[benjamingr]

I'm running the code in a .net 4.5 environment both in Azure and locally.

I'm getting the issue both on Azure and using the Azure emulator when I'm running in .net 4.0 as well

[jamietre]

I just pushed a new branch called pathkey which changes the index to use ushort arrays instead of strings. If this problem is the equality comparer issue, this should resolve it. Give this a try and let me know if it works.

[jamietre]

Did you get a chance to try this? I am interested to know if this was the problem!

[benjamingr]

I'm sorry but this is a work project and I only get to work on it on Tuesdays ( :/ ) which means I won't have an answer until next Tuesday.

[jamietre]

Ha, no worries. Sometimes when people get their problem solved I don't hear from them again - since this has been a longstanding problem I'm just looking forward to (hopefully) hearing it's fixed!

[benjamingr]

Just wanted to say I'm finally getting around to testing this today, I'll let you know what the results are

I got the source and compiled it, so far so good. Ran regression normally, everything seems ok,it now ran over the first 1000 urls and it looks promising :)

Update, meanwhile regression doesn't pass on the current URLs (this might be a change from 1.3.1 to 1.3.3 I'm not sure) on the good side, it doesn't crash the AZURE simulator so that's a good sign 👍

Update 2: Issue was due to some odd change in the way comments were treated between CQ 1.3.1 and CQ 1.3.3 . (Or maybe somewhere in my own code) I just filtered by passing DOM render options to the Render method.

So far so good, no crashes

[jamietre]

Sounds promising so far! There was a bug related to comments (e.g. I think they were being omitted entirely from either input or output) in one 1.3.x release, that was fixed subsequently. That could be what you were seeing there.

[benjamingr]

Looks good, my code now passes regression, no crashes. I did not check Remove() however it solved this bug when using .Html(). Thanks :) I hope this get merged soon

[jamietre]

Latest is up as a prerelease on nuget. Still working with someone else on another character set issue but everything is passing for me so should be looking good for a release soon.