Usage of setCurrentNodeLocked()

[Dmitry Kazakov]

> For now, I think the systemlocked mechanism is a working option


Well, can you proof it? I mean can you proof it works theoretically, without
running Krita and life-testing it with a couple of mouse clicks? Why do i
ask? Because in multithreaded and asynchronous environment (our threading +
Qt's quasi-asynchronous signals) life-testing can easily show no bugs. As
for me, i can't proof it.

All i can tell about this, it may have really subtle, rare, difficult to
reproduce and catch  bugs, like:
1) Race conditions between threads reading/writing a global variable
KisBaseNode::systemLocked().
2) Race conditions because of quasi-asynchronous signals due to processing
the queue in KoProgressUpdater
3) In it's current state, not all user actions are covered by these locks
(like all the methods from KisImage), so they do not solve the problem. More
than that, i can't even tell what will happen, if you add this to KisImage,
because some of the methods of the image work with several nodes. And, you
know, the place where at least two locks appear is a weird place. And,
taking into account this "lock" is a variable, instead of a stack (a stack
or a counter should be used for a reentrant lock), the state of the variable
can easily become undefined.



> , but as you say, we are doing it in the wrong way. We could have a
> KisNodeLocker object that's created at the start of the option on the stack
> and on deletion (when the action method ends) should unlock the node,
> instead of manually locking and unlocking. That is a relatively small change
> that will help a lot.
>

That may fix only one problem of a set of bugs. We should fix the root of
the problem, but not it's consequences.


> I know you would prefer a queuing mechanism, and I still sort of agree
> desktop Sven's objections, but it needs really careful designing since then
> every non-interactive action (like fill, filter, gradient, scale) needs to
> be encapsulated in a job and the job put on a queue.
>

Yes, but it can be easily proofed and unit-tested, in contrary to the
systemlocked approach.

And there would still be a clash with the paint tool, since I don't think
> that its actions should be mixed in the queue with the long running actions.
> And it might still be better usability to disallow starting a new gradient
> while the old one is running, for instance.
>

I think there is nothing bad in adding all the tools and actions to the same
queue in case the queue has good barrier abilities.


PS:
Btw, our swapping mechanism started to work only a semi-formal proof. The
proof that it will not block and all the operations will be performed in a
finite number of steps. Before that it was constantly crashing and hanging
up.


-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20100921/16af1174/attachment.htm