Thread related crash on failed dive computer download

Dirk Hohndel dirk at hohndel.org
Tue Oct 31 12:41:32 PDT 2017


On Tue, Oct 31, 2017 at 08:10:32PM +0100, Berthold Stoeger wrote:
> > On Tue, Oct 31, 2017 at 03:52:28PM +0100, Berthold Stoeger wrote:
> > > As a FYI, the patch attached, though without commit-message because it is
> > > simply too ugly. It works for my particular test case, but...
> > 
> > Why is it too ugly?
> 
> Duplication of the va_args thing. At a place where it doesn't belong.

Shrug.

> Movement of C-style strings to QString back to C-style strings back to 
> QStrings. Oh my.

Yes, that always bothers me when we transition through C and C++ code and
back. We should try to keep them as just C strings for as long as
possible.

> No encapsulation whatsoever. C function defined in C++ header and declared 
> extern in C source.
> Etc. :(

Shrug. I'm trying to get a last minute fix for a bug that I introduced...

> > > +	errorList.setLocalData(QStringList());
> > 
> > So we initialize to an empty list. I'm unclear if there could be messages
> > still in that local storage that haven't been delivered, yet.
> 
> You're right, the local storage should be gone once the thread exits, even 
> though the thread object is reused.
> 
> What should be cleared though is the "errors" member, because the object is 
> reused.
> 
> BTW: Another bug: the code doesn't clear the membuffer.

Oops.

> Forget the std::move - it should be removed. This would be used in standard C+
> + where list assignment makes a deep copy. A std::move on the other hand just 
> replaces pointers. But as I just learned, Qt containers do copy on write, so 
> this is not necessary. I have no idea how they would react to a std::move.

OK

> > > +// Hack! for report_error_thread()
> > > +#include "core/downloadfromdcthread.h"
> > 
> > Why is that a hack?
> 
> Because downloadfromdcthread.h should not be needed here?

This should go into a helper .h file.

> > > @@ -220,6 +220,9 @@ void DownloadFromDCWidget::updateState(states state)
> > > 
> > >  	// got an error
> > >  	else if (state == ERROR) {
> > >  	
> > >  		timer->stop();
> > > 
> > > +		foreach(const QString &s, thread.errors) {
> > > +			report_message(s.toUtf8().data());
> > > +		}
> > 
> > This one I don't understand... why aren't these reported when we do the
> > std::move() above?
> 
> On completion, the thread moved the messages collected in the thread local 
> area to the "errors" member of the thread object. And here the main thread 
> displays the collected messages.
> 
> To be honest, apart from the errors mentioned above, I'm not even sure that 
> the idea behind the code is sound. If you want, I can send an updated/cleaned 
> up patch, but I have a bad feeling with this one. There probably should at 
> least be a non-varargs version.

The idea that the thread collects its messages and that the caller of the
thread then initiates their display when it is done seems sound to me.

I'd really like a version that's good enough to use in 4.7.2 - and I'm
open to further refinement once that's released.

/D


More information about the subsurface mailing list