Is this a race condition...?

mzimmers

Hi all -

I'm concerned about a bit of code in a file transfer utility:

QObject::connect(m_sock, &QAbstractSocket::readyRead, this, &Widget::socketRead);
...
void Widget::socketRead()
{
    m_socketBytesRead = m_sock->read(m_buffIn, MAX_BUFF_SIZE - 1);
}
...
// in widget processing:
bool b = m_sock->waitForReadyRead(3000);
if (b)
{
	if (m_socketBytesRead > 0)
	{
		// process m_buffIn
	}
}

Could the in-widget test for m_socketBytesRead return before the read in the slot has completed?

Thanks...

dheerendra

I don't see any race here. You are waiting for 3 sec. If data does not come it will proceed as usual as no data has arrived.

JonB

@dheerendra
I don't think that is what he is thinking about. I presume he's asking whether he can guarantee that socketRead() will have executed its m_socketBytesRead = ... before m_sock->waitForReadyRead(3000); returns and his code then executes if (m_socketBytesRead > 0) .... @mzimmers right? Based on absolutely nothing, I would guess that all signals/slots are fired before waitForReadyRead() returns to caller, but it's a reasonable question.

mzimmers

@JonB yeah, that's it exactly. I have two consumers of the signal, and it's not clear that the sequence of processing is predictable.

The more I think about this, the more I think my design sucks...

JonB

@mzimmers
One observation: aren't you Windows? Because if so you are encouraged to avoid waitForReadyRead() anyway, so would suggest redesign. At least you ought look at https://stackoverflow.com/a/44693193/489865 and act on it.

mzimmers

I'm currently developing on Windows, but this app is going to be built to run on a Mac (for Apple the company, as a matter of fact). I've heard of this bug before, but I have yet to experience it. The alternative would be rather ugly: a timer, a wait, and then checking the return code. I'm tempted to try to live with it.

kshegunov

@mzimmers said in Is this a race condition...?:

The alternative would be rather ugly: a timer, a wait, and then checking the return code. I'm tempted to try to live with it.

Why would you have a timer and a wait for something that's asynchronous by design? Can't you just connect to bytesAvailable() and decide whether to read or not?

mzimmers

@kshegunov my understanding is that bytesAvailable() is a function, not a signal, so I don't know how I'd connect to it.

I can see, though, that this is really a lousy design. I'm performing a file transfer synchronously from a slot. I think I need a separate thread to handle the socket IO, and a whole bunch of connections between it and my widget.

jsulm

@mzimmers I don't see a need for a thread. Simply use asynchronous APIs instead of waiting.

JonB

@mzimmers
Far be it for me to risk interpreting what my learned colleague @kshegunov is getting at. But...

As he & @jsulm are suggesting, why is it that your architecture requires socketRead() to set a variable and exit and your widget processing code waits for that with waitForReadyRead()? This is "synchronous" architecture. The encouraged pattern is for your readyRead() slot to read the bytes and do whatever with them itself, perhaps raising a signal itself at the end to inform the widget. There is then no "waiting" for anything. This is "asynchronous" architecture. You can do all this in a single thread, no need for you to create one of your own.

VRonin

Is this a race condition?

It can't be a race condition unless you have multiple threads. Do your objects all live in the same thread?

@mzimmers said in Is this a race condition...?:

I think I need a separate thread to handle the socket IO

No, just move // process m_buffIn inside void Widget::socketRead()

mzimmers

@VRonin I only have one thread. Maybe "race condition" isn't the correct term, but I don't know whether I can depend on whether the socketRead() slot will update the member variable m_socketBytesRead before my test of that variable in the widget.

As background information, the feature I'm working on is transferring an Intel Hex file to an embedded device. The file's about 200K and the sockets are restricted to 1K, so there's a fair number of writes. After each write, I wait for an ACK before I send the next write.

kshegunov

@mzimmers said in Is this a race condition...?:

Maybe "race condition" isn't the correct term

It's not, you'd call that "slot interleaving" (due to the event loops being spun inside some slot), however this is beside the point.

After each write, I wait for an ACK before I send the next write.

What you describe is a stateful communication. You do that not by waiting but by managing a state machine. If this is TCP, then this ACK is really superfluous, the TCP stack does that for you behind the scenes.

mzimmers

@kshegunov nice explanation...thanks. I've re-written the code so that the signal from the button opens the file to be transferred and performs an initial write. A slot catches the ACK and sends another chunk from the file. Much nicer now.

As an aside, the purpose of the ACK in this application isn't just to acknowledge receipt of a valid message, but to inform the sender that the receiver is ready for another message. The receiver is an 8-bit AVR device that is saving the file to a slow EEPROM, so the ACK is needed to prevent the sender from overwhelming the receiver's meager resources.