[Enig123]

In ListenSocket.cpp line 585-588:

					if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
						newDS = DS_ONQUEUE;
					else
						newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE

should be

					if (!creqfile->IsStopped()) {
						if  (creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
							newDS = DS_ONQUEUE;
						else
							newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE
					}

to behave in exact way as previous 0.50b version.

Edit: There's another spot in the same file with the same code refactoring that needs to be addressed similarly.

This post has been edited by Enig123: 28 August 2024 - 01:16 AM

[fox88]

I am quite sure that a while ago you suggested to change DS_DONE to DS_ONQUEUE in some places.
Do you think now that here this might cause incorrect behaviour?

This post has been edited by fox88: 28 August 2024 - 07:52 AM

[Enig123]

fox88, on 28 August 2024 - 12:52 AM, said:

I am quite sure that a while ago you suggested to change DS_DONE to DS_ONQUEUE in some places.
Do you think now that here this might cause incorrect behaviour?

Not related I think. The DS_CONNECTED status is only used as an indicator and never be actually applied here.

[fox88]

There was no question about DS_CONNECTING. What combination of conditions leads to incorrect state change?

[Enig123]

fox88, on 28 August 2024 - 10:43 AM, said:

There was no question about DS_CONNECTING. What combination of conditions leads to incorrect state change?

If creqfile->IsStopped() is true, your codes would not send cancel transfer signal which it should.

[fox88]

Agreed.
Thanks for the report.

[BuyukBang]

	case OP_SENDINGPART:
		{
			// see also OP_SENDINGPART_I64
			if (thePrefs.GetDebugClientTCPLevel() > 1)
				DebugRecv("OP_SendingPart", client, (size >= 16) ? packet : NULL);
			theStats.AddDownDataOverheadFileRequest(16 + 2 * 4);
			client->CheckHandshakeFinished();
			EDownloadState newDS = DS_NONE;
			const CPartFile *creqfile = client->GetRequestFile();
			if (creqfile) {
				if (!creqfile->IsStopped() && (creqfile->GetStatus() == PS_READY || creqfile->GetStatus() == PS_EMPTY)) {
					client->ProcessBlockPacket(packet, size, false, false);
					if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
						newDS = DS_ONQUEUE;
					else
						newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE
				}
			}
			if (newDS != DS_CONNECTED && client) { //client could have been deleted while debugging
				client->SendCancelTransfer();
				client->SetDownloadState(newDS);
			}
		}
		break;

bool CUpDownClient::CheckHandshakeFinished() const
{
	if (m_bHelloAnswerPending) {
		// 24-Nov-2004 [bc]: The reason for this is that 2 clients are connecting to each other at the same time.
		//if (thePrefs.GetVerbose())
		//	AddDebugLogLine(DLP_VERYLOW, false, _T("Handshake not finished - while processing packet: %s; %s"), DbgGetClientTCPOpcode(protocol, opcode), (LPCTSTR)DbgGetClientInfo());
		return false;
	}

	return true;
}

CheckHandshakeFinished has nothing to do here. There are a lot of similar usage in ListenSocket.

This post has been edited by BuyukBang: 29 August 2024 - 10:49 AM

[BuyukBang]

Enig123, on 28 August 2024 - 04:02 AM, said:

In ListenSocket.cpp line 585-588:

					if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
						newDS = DS_ONQUEUE;
					else
						newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE

should be

					if (!creqfile->IsStopped()) {
						if  (creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
							newDS = DS_ONQUEUE;
						else
							newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE
					}

to behave in exact way as previous 0.50b version.

Edit: There's another spot in the same file with the same code refactoring that needs to be addressed similarly.

"!creqfile->IsStopped()" check is already included in upper if block, so "!creqfile->IsStopped()" is always true inside that part. Then your suggestion and the current 0.70b are logically equal.

			if (creqfile) {
				if (!creqfile->IsStopped() && (creqfile->GetStatus() == PS_READY || creqfile->GetStatus() == PS_EMPTY)) {
					client->ProcessBlockPacket(packet, size, false, false);
					if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_PAUSED || creqfile->GetStatus() == PS_ERROR)
						newDS = DS_ONQUEUE;
					else
						newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE
				}
			}

This post has been edited by BuyukBang: 29 August 2024 - 11:20 AM

[Enig123]

BuyukBang, on 29 August 2024 - 04:12 AM, said:

"!creqfile->IsStopped()" check is already included in upper if block, so "!creqfile->IsStopped()" is always true inside that part. Then your suggestion and the current 0.70b are logically equal.

Not 100% sure if it will stand later, technically it could has been altered.

Edit: An example is "Pause when preview possible" enabled.

This post has been edited by Enig123: 29 August 2024 - 07:50 PM

[BuyukBang]

Enig123, on 29 August 2024 - 10:49 PM, said:

BuyukBang, on 29 August 2024 - 04:12 AM, said:

"!creqfile->IsStopped()" check is already included in upper if block, so "!creqfile->IsStopped()" is always true inside that part. Then your suggestion and the current 0.70b are logically equal.

Not 100% sure if it will stand later, technically it could has been altered.

Edit: An example is "Pause when preview possible" enabled.

No, it's not possible in this case. This is all happening in the main thread, and we're talking about consequent lines.

[fox88]

Enig123, on 29 August 2024 - 12:48 AM, said:

If creqfile->IsStopped() is true, your codes would not send cancel transfer signal which it should.

After a bit of thinking, the code may be:

client->ProcessBlockPacket(packet, size, false, false);
if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_ERROR)
	newDS = DS_ONQUEUE;
else
	newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE

Other cases should have been handled already.

[Enig123]

fox88, on 30 August 2024 - 02:36 AM, said:

After a bit of thinking, the code may be:

client->ProcessBlockPacket(packet, size, false, false);
if (!creqfile->IsStopped() && creqfile->GetStatus() == PS_ERROR)
	newDS = DS_ONQUEUE;
else
	newDS = DS_CONNECTED; //any state but DS_NONE or DS_ONQUEUE

Other cases should have been handled already.

To keep the codes as simple and straightforward as possible, that would be my preference.

Implying or omitting will have more chance that would lead to potential bugs.

[fox88]

Code correctness is mandatory; aesthetics - optional and subjective.