PDFDocument.SaveToMetaData() will crash spuriously

[GerHobbelt]

Running the app at a machine load of sustained 100% (thanks to Qiqqa OCR background processes).

The crash occurs when you edit the BibTeX of a document in the Sniffer, but CPU load is so high that the responsiveness is reduced, so by the time Qiqqa has queued the document for storage in a background task (passing an implicit object reference of pdf_document) your next keypress == user edit comes in and it can happen that the Write action coincides such that .NET internals will cause the JSON serializer to barf because "the list has changed" -- which it did!

[EDIT] in my case it was the annotations list, but PDFDocument has several List<string> amd the error is very time-dependent hence hard to reproduce. Anyway, the key point here is that in a multithreaded environment, one thread Add or Remove-ing to a List while another thread does a List walk (via the iterator, obviously), then .NET internals MAY detect this modifying-while-sequencing anomaly and barf a hairball.

Either we throw critical sections all over the place or we change the application such that it passes a COPY of the pdf_document across thread boundaries.

Re that lingering user edit due to slow responsiveness: that SHOULD be dealt with already by yet another Save instruction being queued in that same thread-crossing queue as before.

[EDIT] what I meant here is: will that parallel (and possibly shortly afterwards completed) edit be recognized as yet another change and trigger a store-to-disk action again? AFAICT Qiqqa SHOULD cope with this correctly, except there's a timestamp involved that doesn't guarantee a difference detect when set follows reset within the same time unit (1/100th of a second? 1ms? OS dependent? The time-marker should be critical sectioned and produce an identifiable increment for every invocation, so that even edge cases such as this scenario will get detected properly. #97)

Hence at least SaveToMetaData() has to be made thread-safe by copy-instancing the pdf document and the metadata therein, i.e. a (semi)DEEP COPY is called for AFAICT.

[GerHobbelt]

The crashes in Qiqqa (as reported in this issue and others, e.g. #98) are at least in part due to thread-unsafe coding of the PDFDocument class, while references to PDFDocument instances are poassed to multiple threads and async event handlers.

After some contemplation, it was decided to make PDFDocument threadsafe by introducing mandatory lock()ing inside every public interface.

To ensure serialization and regular within-thread larger code thunks working on a PDFDocument instance is still performant and backwards compatible, the original PDFDocument class was moved into a "ThreadUnsafe" namespace and renamed to PDFDocument_ThreadUnsafe so every bit that uses this unsafe codebase directly will be utterly aware that it is doing so. The new PDFDocument implementation is a wrapper (facade pattern) around this old unsafe code, while the new PDFDocument API is made threadsafe by properly applying lock-ing everywhere.

Remaining suspect areas

Inks, Annotations and Highlights still pass their lists by reference to callers instead of (deep) copying, hence these bits are still thread-unsafe while the code in PDFDocument might look safe there as there's a basic lock() provided, but as anyone savvy in multiprocess coding knows, that cute critical section is not enough.

This is a TODO that should be addressed in a separate issue.

[GerHobbelt]

Closing ^{and decluttering the issue list so it stays workable for me}: fixed in https://github.com/GerHobbelt/qiqqa-open-source mainline=master branch, i.e. v82pre3 experimental release and beyond.