Review Request 108385: Reduce risk of timeout and race condition in KPtyProcessTest

[Oswald Buddenhagen]

> On Jan. 13, 2013, 6:45 p.m., Oswald Buddenhagen wrote:
> > and why exactly do you sleep instead of looping with waitforreadyread?
> 
> Jon Severinsson wrote:
>     Because that would be an (almost) busy-loop (there are already *some* data, so waitForReadyRead could return before the timeout).
> 
> Oswald Buddenhagen wrote:
>     right. my (kpty) implementation waits for *any* data to be available (which is (or was) consistent with something i don't remember), while thiago's (qsocket & co.) implementations wait for *more* data to be available. this really should be made consistent at some point ... at this point i think thiago's interpretation is more useful, even if it means writing more code in the common case.
> 
> Jon Severinsson wrote:
>     I have not looked too closely at either implementation, and can't say I really understand how either actually work, so this might be pointless, but I would still want to give a warning about the whole "waiting for *more* data" (as opposed to "waiting for *some* data") concept, as that immediately brings to question "more since *when*?".
>     
>     More data since the waitForReadyRead() call *can* *not* be used correctly. Never under any circumstance!  Every call would be a race condition (all data that is ever going to come might have come just before you made the call), and would at best result in an unnecessary sleep of "timeout" ms, and at worst result in a complete deadlock, or if the rest of the system conspires against you, an infinite loop.
>     
>     More data since last "relevant" API call prior to waitForReadyRead() could work, but defining "relevant", and getting both the implementation and documentation right would likely be a nightmare, and chances a API consumer makes a mistake is still possible, even likely, so this is not something I would recommend.
>     
>     So imho "waiting for *some* data" is the only right thing to do, even though it results in this ugly code in some cases...

no. the actual querying of the data source is synchronous as far as threading is concerned: you either do it via waitFor*() or by returning to the event loop. consequently, code like "if (!dev->bytesAvailable()) dev->waitForReadyRead();" is entirely well-defined. the waitFor*() functions are thus defined to mean "synchronously wait for the emission of the likewise named signal".


- Oswald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108385/#review25394
-----------------------------------------------------------


On Jan. 13, 2013, 1:03 p.m., Jon Severinsson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108385/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2013, 1:03 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
> Increase timeout, and sleep a while after waitForReadyRead() returns,
> as it only guarantees that *some* data is available to read, while
> the test assumes that a full line of data is available to read...
> 
> This reduces failure rate from 10% to 2% on my setup.
> 
> 
> Diffs
> -----
> 
>   kpty/tests/kptyprocesstest.cpp b95ae26 
> 
> Diff: http://git.reviewboard.kde.org/r/108385/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jon Severinsson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130114/67da051d/attachment.html>