Re: possible data corruption while scrolling through rows

On Monday 17 January 2005 15:47, abel deuring wrote:
> Mike Richardson wrote:
> > On Sunday 16 January 2005 23:22, abel deuring wrote:
> >>hi,
> >>
> >>I noticed the following bug in Rekall (I'm using version 2.2.3):
> >>
> >>If a form block has a sub block, it can happen that the contents of some
> >>rows of the sub block is deleted, when the user presses for example the
> >>up or down arrow quickly two or more times in the parent block. (The
> >>problem may perhaps also occur for a simple form block, if row updates
> >>take a long time -- I didn't read the sources carefully enough to tell
> >>for sure...)
> >>
> >>At some time during a call of KBFormBlock::doOperation e.g. for a down
> >>arrow keystroke, a progress bar is opened, and TKProgress::setDone calls
> >>qApp->processEvents. If the next key stroke arrives in the event queue
> >>before this call, processEvents causes another call of
> >>KBFormBlock::doOperation. The problem now is that
> >>KBFormBlock::doOperation is not reentrant safe. It happened to me quite
> >>often, that the first call of KBFormBlock::doOperation was at the point,
> >>where KBControl::clearValue() has been called, but where
> >>KBControl::getIniValue returns a non-empty value. Hence, during the
> >>second call of doOperation, KBControl::changed returns true, and all
> >>values of the affected row are deleted.
> >>
> >>I fixed this (more or less...) by not allowing recursive calls of
> >>doOperation. See the attached patch. It detects recursive calls and
> >>queues them so that the "real execution" can be deferred until the first
> >>call of doOperation is finished. This may lead to an unexpected
> >>behaviour, because the execution sequence for events can be changed, if
> >>events are queued for different block at the same time. Any comments?
> >
> > Wow! That is seriously subtle! I'll see about getting the fix in (tho' I
> > think I might use a QValueList rather than a linked list).
>
> Right, a QValueList is of course more "QT-ish" ;) Another possible issue
> is the integer field inOperation. If some future version of Rekall will
> use threads, using QSemaphore instead would be more robust.
>
> > Can you describe the circumstances that the progress bar can appear? I'm
> > curious in case there are some other places that this might arrise.
>
> TKProgress::setDone is called by KBQryLevel::InsertRC and by
> KBCopyExec::showProgress.

Ah, yes, I'd forgotton that

>
> But qApp->processEvents is called by more methods:
> KBDumper::dumpDetails, KBDumper::slotTimer, KBDumper::showProgress,
> KBLoader::loadDetails, KBLoader::slotTimer, KBLoader::showProgress,
> KBCopyExec::showProgress, KBErrorDlg::slotDetails,
> KBConductor::snapshot, TKProgress::setDone, TKTextEditor::print and
> TKTextEditor::printPreview.
>
> <comment type="back seat driver">
> I think the main question is, on which "level" qApp->processEvents
> is/should be called, and/or if/how events that can't be processed
> immediately can be queued for later execution.
>
> Of course, a decent program must handle user input and other events as
> quickly as possible, and some actions like database queries can take a
> long time, hence it makes sense to call qApp->processEvents from time to
> time. OTOH, making them reentrant safe can be quite complex, so I am
> wondering, if of how is it possible to check, if certain events can be
> executed immediately, or if it should queued for later execution. While
> my patch does just that, but only "locally", based on a symptom; but I
> think it is possible that other parts of Rekall might have similar
> reentrance problems, so it would perhaps be better to check for possible
> reentrance problems at a more centralized location.

My first thoughts are, what we are interested in is not QT events themselves - 
since that includes stuff like repaints - but "rekall" events, which would be 
stuff like keystrokes and button clicks. Keystokes either update values in 
controls, which should not be a problem unless the change triggers a "rekall" 
event, in which case they are in the same class as, say a button click.

In that case there are two cases. Keystrokes get fed through the KBNavigator 
class, so could probably be serialised there (ie., if KBNavigator gets a 
keystroke while it is processing a previous one, then it is queues); events 
like button clicks end up in an instance KBEvent (for the event they are 
triggering) which again gives a serialisation point. Actually, I guess that 
the two streams should be merged, probably at the form level. I hate this 
sort of stuff, its a real PITA to test (and to reproduce any problems !)

At some point - when Rekall gets around to supporting asynchronous transfers 
from databases - then it will have to be multithreaded, so, yes, the whole 
issue becomes more critical.

Gonna have to think about this one!

>
> This sounds quite vague, but I am not familiar enough with QT's event
> handling concept to write something more specific...
> </comment>
>
> Abel

Regards
Mike


> _______________________________________________
> Rekall-devel mailing list
> [email protected]
> http://www.mailman.a-i-s.co.uk/cgi-bin/mailman/listinfo/rekall-devel