2

Some background for this question is my previous question: non-member function pointer as a callback in API to member function (it may well be irrelevant).

The callback launches a thread that writes some data. There is another thread that reads the same data, and that results in some crashes. I just took a crash course in multi-threading (thanks SO), and here is my attempt to guarantee that the data isn't accessed by the writer and the reader at the same time. I'm using some mutex mechanism from Qt (QReadWriteLock).

#include <QSharedPointer>
#include <QReadWriteLock>

Class MyClass
{
public:
  MyClass();
  bool open();
  float getData();
  void streamCB(void* userdata);

protected:
  float private_data_;
  QSharedPointer<QReadWriteLock> lock_;
};

// callback wrapper from non-member C API to member function void
__stdcall streamCBWrapper(void* userdata)
{
   static_cast<MyClass*>(userdata)->streamCB(userdata);
}

// constructor
MyClass::MyClass()
{
  lock_ = QSharedPointer<QReadWriteLock>(new QReadWriteLock());
  lock_->lockForWrite();
  private_data_ = getData();
  lock_->unlock();
}

// member callback
void MyClass:streamCB(void* userdata)
{
  float a = getData(); 
  lock_->lockForWrite(); //*** fails here
  private_data_ = a;
  lock_->unlock();
}

I have a segmentation fault while running the program. The VS debugger says Access violation writing location 0x00fedbed. on the line that I marked //*** fails here. The lock worked in the constructor, but not in the callback.

Any idea what goes wrong? What should I look at? (and how can I refine my question) Thanks!

Other relevant thread Cannot access private member declared in class 'QReadWriteLock'Error 1 error C2248: 'QReadWriteLock::QReadWriteLock' (I used the QSharedPointer suggestion)

Edit 1: The callback is set up 

bool MyClass::open()
{
  // stuffs
  int mid = 0;
  set_stream_callback(&streamCBWrapper, &mid);
  // more stuffs
  return true;
}

Edit 2: Thank you for all the suggestions. So my mistake(s) may not be due at all to the mutex, but to my lack of understanding of the API? I'm quite confused.. Here is the API doc for the set_stream_callback.

typedef void (__stdcall *STREAM_CALLBACK)(void *userdata);

/*! @brief Register a callback to be invoked when all subscribed fields have been updated
 *
 *  @param streamCB pointer to callback function
 *  @param userdata pointer to private data to be passed back as argument to callback
 *  @return 0 if successful, error code otherwise
 */
__declspec(dllimport) int __stdcall set_stream_callback(
    STREAM_CALLBACK streamCB, void *userdata);
Community
  • 1
  • 1
Hugues Fontenelle
  • 5,275
  • 2
  • 29
  • 44
  • 1
    Is the callback (`streamCB`) getting called on a valid instance? – Angew is no longer proud of SO Nov 13 '12 at 13:53
  • What would constitute a non-valid one? (sry I did not understand the question) – Hugues Fontenelle Nov 13 '12 at 13:55
  • 1
    I think that @Angew is suggesting that the userdata may not be a valid MyClass pointer - you should maybe check the address with a debugger to see if it's the same as when the callback was set up. – Martin James Nov 13 '12 at 14:03
  • @MartinJames Yes, that's what I meant. – Angew is no longer proud of SO Nov 13 '12 at 14:08
  • Oh - and also make sure that the MyClass instance lifetime lasts until the callback is executed. C++ developers have a nasty habit of creating objects on stacks and expecting them to still be around when some other thread makes a callback later. – Martin James Nov 13 '12 at 14:12
  • **1.** I edited the streamCB method in my question. userdata is still valid (I checked with the debugger, thanks for sharing this technique) **2.** the class is still alive. – Hugues Fontenelle Nov 13 '12 at 14:16
  • OK, what happens if you comment out the 'lock_->lockForWrite()' and the 'lock_->unlock();'. Does it still crash immediate? – Martin James Nov 13 '12 at 14:58
  • Please provide a "working" example or at least show some calling code (creation of MyClass instance, setting up the callback, etc) – Johannes S. Nov 13 '12 at 15:42
  • @Martin James: It does not crash immediately, only when my data gets corrupted. In some case my application can run a long time without crashing but the data values are not correct. Which is why I started to implement this mutual exclusion. – Hugues Fontenelle Nov 13 '12 at 15:42
  • @Johannes S. Difficult to isolate the creation of the class in the code, but I edited the Q about how the callback is set up. Tx – Hugues Fontenelle Nov 13 '12 at 15:56
  • 3
    @Hugues - forgive me if I'm missing something, but should the callback setup not have a 'this' parameter? You seem to be passing the 'mid' int data member, (or the address of it, at least:). – Martin James Nov 13 '12 at 16:07
  • @Martin James. The first callback parameter is a pointer to a non-member function. The second is a reference to an int but somehow I can put pretty much anything here.. Well, gotta study that a bit more closely, thanks. – Hugues Fontenelle Nov 13 '12 at 16:38
  • 1
    The free 'streamCBWrapper' function must get a reference to the MyClass instance to call methods on it. The only way it can get it is via the parameter. Replace '&mid' with 'this'. – Martin James Nov 13 '12 at 16:52
  • 1
    @Hugues So are you sure that you can pass anything and that the parameter is unused inside `set_stream_callback` except for being passed to you callback function as a `void*` ? If so, I would expect `set_stream_callback` to take a `void*` as second argument as well... – Johannes S. Nov 13 '12 at 17:05
  • My fault for not being able to translate the C API into C++. Now I know. Thank you. – Hugues Fontenelle Nov 13 '12 at 19:01

2 Answers2

1

Good example why sufficient code example is required.

If I interpret your callback syntax correctly,

set_stream_callback(&streamCBWrapper, &mid);

sets streamCBWrapper as callback function, while &mid is the pointer to userdata.

In the callback, you are actually now casting a pointer to int to MyClass, then try to access a member variable of a non-existant object.

Make sure to pass an instance of MyClass to your callback. I assume this would be this in your case.

Johannes S.
  • 4,566
  • 26
  • 41
  • 1
    Yeah - that's what I thought. – Martin James Nov 13 '12 at 16:54
  • Thank you all Johannes S., Martin James and Angew! I'm impressed that you all realized that the mutex was not the issue here.. I corrected the 'this'. And now whatever lock I want to put on the data works :-) – Hugues Fontenelle Nov 13 '12 at 18:59
  • @JohannesS. - OK, no problemo. I wasn't confident enough to give an answer 'cos I thought the possibilty of an incrrect pointer had been elimiated by some earlier debugging :( – Martin James Nov 13 '12 at 19:15
1

Sounds fundamentally like a threading issue to me. Since you're using the Qt mutexing anyway, you might consider using Qt's threading mechanisms and sending signals and slots between the threads. They're pretty well documented and easy to use as long as you follow the suggestions here and here.

Phlucious
  • 3,704
  • 28
  • 61