BHoM_Engine: ThreadStatic attribute taken off the debug log and replaced by a lock

[pawelbaran]

Issues addressed by this PR

Closes #2009
Closes #2745

Test files

Not sure how to test it meaningfully - the code still works as previously and does not show any signs of issues. I think that the best way of tasting can be simply merging the change and watching the logs carefully for the coming 2 sprint to make sure that this does not break anything.

Changelog

Additional comments

The approach taken has been discussed with @IsakNaslundBh: it seems to deliver best value for price:

• the log sits safely behind the lock
• the lock is Global, so it can be used across the entire project
• Query.DebugLog has already been internal so there is no risk of the collection being modified from the outside

An alternative that we discussed with @IsakNaslundBh was moving BH.oM.Base.Debugging.Log to the engine's Objects and encapsulating it properly. However, this would require more effort that does not seem to be justified for now.

[pawelbaran]

@BHoMBot check compliance
@BHoMBot check null-handling

[bhombot-ci]

@pawelbaran to confirm, the following checks are now queued:

• code-compliance
• documentation-compliance
• project-compliance
• branch-compliance
• dataset-compliance
• copyright-compliance
• null-handling

[pawelbaran]

@BHoMBot check compliance

[bhombot-ci]

@pawelbaran to confirm, the following checks are now queued:

• code-compliance
• documentation-compliance
• project-compliance
• branch-compliance
• dataset-compliance
• copyright-compliance

[pawelbaran]

@BHoMBot check core
@BHoMBot check null-handling
@BHoMBot check serialisation
@BHoMBot check versioning
@BHoMBot check installer

[bhombot-ci]

@pawelbaran to confirm, the following checks are now queued:

• core
• null-handling
• serialisation
• versioning
• installer

[adecler]

I am happy with the solution.
Agreed that it is best to keep things simple and leave the log where it is for now.

[pawelbaran]

@BHoMBot check ready-to-merge

[bhombot-ci]

@pawelbaran to confirm, the following checks are now queued:

• ready-to-merge

[IsakNaslundBh]

Looks good to me. All instances using the m_DebugLog or calling the DebugLog() method are protected with the same lock.

Looks safe to me as long as the DebugLog() method stays internal as stated in the PR description by @pawelbaran .

In terms of testing, could be done with a dummy method with a Parallel.ForEach or similar simply raising tons of events and making sure no race conditions occurs. From the code side I can not see that being an issue, but ofc, good to test before merging.

[pawelbaran]

Thanks @adecler @IsakNaslundBh. Following the request, I am pasting the code that I have used to (simply) test the feature, with positive results:

int errorCount = 10000;
BH.Engine.Base.Compute.ClearCurrentEvents();
Parallel.ForEach(Enumerable.Range(0, errorCount), i => { BH.Engine.Base.Compute.RecordError($"Error no. {i + 1}"); });

var errors = BH.Engine.Base.Query.CurrentEvents();
foreach (var e in errors)
{
    Console.WriteLine(e.Message);
}

Console.WriteLine($"\nTotal error count: {BH.Engine.Base.Query.CurrentEvents().Count}, test finished with result: {errors.Count == errorCount}");
BH.Engine.Base.Compute.ClearCurrentEvents();

I will merge the PR now, thanks all 👍