Bug 59927

Created attachment 73725 [details] [review]
okular testcase

And here my okular testcase.
@Adam: I allow me to add You to the CC list. One of the reasons is that I can't reconstruct the problems with Page::search, okular uses Page::textList instead of it, and that was already thread-safe with commit of bug 50992. If You still get problems with Page::search, can You provide a testcase for it or tell me, how I can reconstruct it?

(In reply to comment #1)
> Created attachment 73725 [details] [review] [review]
> okular testcase
> 
> And here my okular testcase.
> @Adam: I allow me to add You to the CC list. One of the reasons is that I
> can't reconstruct the problems with Page::search, okular uses Page::textList
> instead of it, and that was already thread-safe with commit of bug 50992. If
> You still get problems with Page::search, can You provide a testcase for it
> or tell me, how I can reconstruct it?

Thanks for keeping me in the loop! As you already pointed out on the mailing list, changing the call to displayPage using the TextOutputDev and hence this patch seems to have solved the search-while-render problem.

Concerning the POPPLER_QT4_THREADSAFE definition, I could personally live without it since I build with defines for the Poppler version given by pkg-config. But I also do not see a problem with it and advertising this feature is probably nice as well.

To round it up and get a definite statement: With this patch applied, I can consider the complete qt4 frontend thread-safe? Meaning that I can remove all locks that protect a document object and its children? (Including life cycle stuff like adding annotations?)

(In reply to comment #2)
> To round it up and get a definite statement: With this patch applied, I can
> consider the complete qt4 frontend thread-safe? Meaning that I can remove
> all locks that protect a document object and its children? (Including life
> cycle stuff like adding annotations?)

Yes, as far as I can see it, all should be thread-safe now. If You find something missing before this bug is closed, add it here, otherwise open a new bug.

(In reply to comment #3)
> (In reply to comment #2)
> > To round it up and get a definite statement: With this patch applied, I can
> > consider the complete qt4 frontend thread-safe? Meaning that I can remove
> > all locks that protect a document object and its children? (Including life
> > cycle stuff like adding annotations?)
> 
> Yes, as far as I can see it, all should be thread-safe now. If You find
> something missing before this bug is closed, add it here, otherwise open a
> new bug.

Thanks for clarifying that. I'll do some more testing then...

Personally i don't think the define makes sense, just depend on the poppler version and that's it.

There's on true that shold be gTrue to be pedantically correct

Why the setoverprint code move? (I think setoverprint should be also gone from globalparams and move to the outputdev itself, this way we can have one with overprint and one without overprint with no fear that it'll collide one with eachother, but that's for the future :D)

It'd be cool it someone could do some silly test with two threads one rendering all the time and the other randomly querying stuff like fonts or adding annotations and run it in a loop to make sure no crash happens (using okular as test case is a bit heavy and difficult)

(In reply to comment #5)
> It'd be cool it someone could do some silly test with two threads one
> rendering all the time and the other randomly querying stuff like fonts or
> adding annotations and run it in a loop to make sure no crash happens (using
> okular as test case is a bit heavy and difficult)

I'll try to create something suitable today.

Created attachment 73759 [details]
test case for multi-threaded access

First try for a test case for stressing multi-threaded document access using Poppler's qt4 frontend.

(In reply to comment #6)
> (In reply to comment #5)
> > It'd be cool it someone could do some silly test with two threads one
> > rendering all the time and the other randomly querying stuff like fonts or
> > adding annotations and run it in a loop to make sure no crash happens (using
> > okular as test case is a bit heavy and difficult)
> 
> I'll try to create something suitable today.

I attached my first attempt at such a test case and currently it crashes in Document::fonts. Actually, sometimes it outright crashes and sometimes there are just tons of error messages, mostly about of unexpected ends of flate streams.

I'll try to enlarge test coverage as we progress.

Thanks a lot for Your test case, Adam. Two reasóns for thanking
a) I learned a little bit more about qt programming. It took me some time to figure out how to compile Your test case, but it wasn't wasted time. I encountered how easy it is to setup threads with qt, perhaps this helps me anywhere else sometimes.
b) Your test case shows me really fast that I missed to fix the FontInfo stuff: it uses the page resource dict which is manipulated during rendering of this page and can't be used by FontInfo during the page is rendered. (BTW, same also occurs when creating / removing annotations, I haven't seen that, but rarely it causes crashes, too)

I was already able to solve these problems, Your test case runs now since 30 minutes without any problems. But before I upload the changes I want change the sources a little bit more according to Albert hints, and because I've now some other dates that have to be wait at least until tomorrow.

Created attachment 73783 [details]
test case for multi-threaded document access

The second attempt to stress test multi-threaded document access in Poppler's qt4 frontend.

(In reply to comment #9)
> I was already able to solve these problems, Your test case runs now since 30
> minutes without any problems. But before I upload the changes I want change
> the sources a little bit more according to Albert hints, and because I've
> now some other dates that have to be wait at least until tomorrow.

Hello Thomas,

I am glad that you liked it. Since the initial problem is solved, I tried to increase the stress level by expanding the test case. This includes using several documents concurrently, more annotation access and a third "crazy" thread which seems to be able to poke out the annotation issues in the current patch as well.

Best regards, Adam.

Created attachment 73820 [details] [review]
fix font info stuff, share annot over threads

(In reply to comment #5)
> Personally i don't think the define makes sense, just depend on the poppler
> version and that's it.

Ok, I removed it from this patch

> There's on true that shold be gTrue to be pedantically correct

Ok, I changed it. But to be pedantically, too, I only correct MY changes, NOT the other uses of true/false :-)

> Why the setoverprint code move? (I think setoverprint should be also gone
> from globalparams and move to the outputdev itself, this way we can have one
> with overprint and one without overprint with no fear that it'll collide one
> with eachother, but that's for the future :D)

Future is there, done.

!!!!!!!!!!!!WARNING!!!!!!!!!!!!
Running Adam's test, I encounterd that there is an "unlock" missing in XRef.cc. This can cause deadlocks with the actual git master, so this part of the patch should be committed asap to avoid bug reports. It will be solved in better way in bug 59933, but I stopped working on it because I think we should close this bug first!

Another comment to this patch: I insert some locks/unlocks in the Annot stuff, because Adam's test case shares annotations over threads and can modify an annotation at the same time in differnt threads. I don't think that this is a normal use case, but nevertheless, better to have it!

Created attachment 73834 [details] [review]
move lock/unlock pairs in Annot to correct place

Sorry if wasting anybodys time: I made some additional tests with Adam's test-case and encountered, that there are problems when drawing annots during rendering pages: the draw routines doesn't use only local variables but modifies also instance variables (appearBuf), and therefore definitely need to be locked!

Here the patch which does it correctly!

Can you rebase the patch? Doesn't apply cleanly anymore

Created attachment 75109 [details] [review]
Rebased patch

Here a rebased patch

Commited, are we done in this one?

P.S: Just had a look at the construct of Page::getResourceDict and i think it's a bit unseasy to use, do you think that in the "cleanup" bug we could instead use two functions? One that always does the copy and one that not? Now you have to keep track if you passed down a non null XRef to know if you have to delete it or not, which i think may be a bit cumbersome and error prone, what do you think?

(In reply to comment #16)
> Commited, are we done in this one?
> 
> P.S: Just had a look at the construct of Page::getResourceDict and i think
> it's a bit unseasy to use, do you think that in the "cleanup" bug we could
> instead use two functions? One that always does the copy and one that not?
> Now you have to keep track if you passed down a non null XRef to know if you
> have to delete it or not, which i think may be a bit cumbersome and error
> prone, what do you think?

I'LL have a look at it abd try to find a better solution in bug 59933.

BTW, during my tests with Adam's program I got one or two times crashes. But the reason for it was the test program itself: two threads fetched the page annots (without any problems) and the deleted the same annotation, the first one wins and the second could crash. I cannot solve that in the library, this is the one case I know where the calling program is responsible to lock two threads against each other.

@Adam: Any known open issues or can we close this bug now?

(In reply to comment #17)
> (In reply to comment #16)
> > Commited, are we done in this one?
> > 
> > P.S: Just had a look at the construct of Page::getResourceDict and i think
> > it's a bit unseasy to use, do you think that in the "cleanup" bug we could
> > instead use two functions? One that always does the copy and one that not?
> > Now you have to keep track if you passed down a non null XRef to know if you
> > have to delete it or not, which i think may be a bit cumbersome and error
> > prone, what do you think?
> 
> I'LL have a look at it abd try to find a better solution in bug 59933.
> 
> BTW, during my tests with Adam's program I got one or two times crashes. But
> the reason for it was the test program itself: two threads fetched the page
> annots (without any problems) and the deleted the same annotation, the first
> one wins and the second could crash. I cannot solve that in the library,
> this is the one case I know where the calling program is responsible to lock
> two threads against each other.

I agree, as is stated in the documentation, the caller is responsible that no more objects refer to a specific annotation before calling "Page::removeAnnotation", so this is invalid usage, even if it is done in a single thread.

> @Adam: Any known open issues or can we close this bug now?

Nothing comes to mind.

Ok, closed then