assert slave command finality
ClosedPublic

Authored by sitter on Thu, Aug 22, 11:19 AM.

Details

Summary

this changes existing qwarnings in state verification logic to be assertive
in order to crash misbehaving slaves so git users can report bugs about
them and we can fix them.
release builds are not affected and will continue to not crash albeit
without warnings. I am not sure it's worth the effort to assert and qwarn.

additionally when no finality was expected we no longer verify this after
the fact but instead at the time finished/error gets called. this, combined
with the assert aborting, has the advantage that we'll see the actual
call chain that resulted in the unexpected finality call. it's still not
ideal since we'd also want to know the previous caller but unfortunately
we have no way to get that reliable (e.g. backtrace() is woefully incapable
of resolving symbols from the slave - probably because it is a module).

lastly, force a misbehaving slave into finished state when it
should have been finished or error'd but didn't. this prevents slaves
getting stuck ad infinitum on account of them not finshing.

Test Plan

misehaving ftp slave now crashes

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Thu, Aug 22, 11:19 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptThu, Aug 22, 11:19 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Thu, Aug 22, 11:19 AM

I see those warnings often, so I'm a bit worried that this will make kioslaves quite crashy for KDE developers for a while, including those unable to fix those issues.

At least we need to fix known issues first, like kio_ftp.

How about we both (and ZaWertun) run with this for a while (and fix things) before merging it upstream?

Note that we could also just export QT_FATAL_WARNINGS=1 before kdeinit5 :-)

src/core/slavebase.cpp
208

typo: Release

209

typo: verification

486

should be %1, not %2

Yeah, it's certainly a very aggressive change. I also think we need this to be outside kdeinit5, otherwise unit tests won't crash since they generally should fork directly. That being said, as a first step maybe we can have this enabled on the CI and see if existing unit tests start failing.

Additionally here's two "transient" options I'd propose:

  • only enable the asserts for Debug builds from git. that way if stuff crashes a lot, the user/developer can expect a quick turn around on the fix
  • give an env var or cmake option that can opt out of the assertions but still run in debug mode? (the cmake option seems a bit pointless, if you run a Debug build you should get debuggy behavior IMHO).
sitter updated this revision to Diff 64744.Tue, Aug 27, 2:25 PM
  • wrap in custom assert defines that either assert or qwarn based on a cmake option
  • new cmake option KIO_ASSERT_SLAVE_STATES enables the asserts. the option is only on by default when run on jenkins
  • fix a bunch of typos

the long term plan here is still to always enable the assertions (conditional on build type anyway), to not break everyones systems the cmake option allows a few brave souls to opt into assertion. also by running jenkins slaves with assertions we'll have additional chances of finding bugs there

dfaure requested changes to this revision.Wed, Aug 28, 7:04 AM
dfaure added inline comments.
src/core/slavebase.cpp
67

Don't you mean if(!cond) { qCWarning... } ? Otherwise the warning will be shown every time, no matter what the condition says.

Or more precisely,

do { if (!(cond)) qCWarning(KIO_CORE) << what; } while (false)

for the usual brace nesting problem.

This revision now requires changes to proceed.Wed, Aug 28, 7:04 AM
sitter updated this revision to Diff 64826.Wed, Aug 28, 11:19 AM

some days one has to wonder how I manage to put on shows. fix warning branch as per David's comments to actually do nothing when the cond was matched

dfaure accepted this revision.Wed, Aug 28, 1:58 PM

You put on shows? On TV? :-)

This revision is now accepted and ready to land.Wed, Aug 28, 1:58 PM

Shoes! I meant shoes! 😿

This revision was automatically updated to reflect the committed changes.

LOL, so even the joke (about you writing buggy code) was buggy, good one ;)

This comment was removed by bcooksley.
This comment was removed by bcooksley.
bcooksley removed a subscriber: fsitter.