Skip to content

Commit

Permalink
Revert "- Removed timeout from TransferSession::StartTransfer() and T…
Browse files Browse the repository at this point in the history 
…ransferSession::WaitForTransfer()"

This reverts commit 7a4061a.
  • Loading branch information
@isiu-apple
isiu-apple committed Apr 15, 2022
1 parent 3541d4a commit 3a9868e
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 27 deletions.
3 changes: 2 additions & 1 deletion src/app/clusters/ota-requestor/BDXDownloader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ CHIP_ERROR BDXDownloader::SetBDXParams(const chip::bdx::TransferSession::Transfe

// Must call StartTransfer() here to store the the pointer data contained in bdxInitData in the TransferSession object.
// Otherwise it could be freed before we can use it.
ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData));
ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData,
/* TODO:(#12520) */ chip::System::Clock::Seconds16(30)));

return CHIP_NO_ERROR;
}
Expand Down
4 changes: 2 additions & 2 deletions src/protocols/bdx/BdxTransferSession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ void TransferSession::PollOutput(OutputEvent & event, System::Clock::Timestamp c
mPendingOutput = OutputEventType::kNone;
}

CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData)
CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

Expand Down Expand Up @@ -177,7 +177,7 @@ CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitD
}

CHIP_ERROR TransferSession::WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
uint16_t maxBlockSize)
uint16_t maxBlockSize, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

Expand Down
8 changes: 6 additions & 2 deletions src/protocols/bdx/BdxTransferSession.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param initData Data for initializing this object and for populating a TransferInit message
* The role parameter will determine whether to populate a ReceiveInit or SendInit
* @param timeout The amount of time to wait for a response before considering the transfer failed
* @param curTime The current time since epoch. Needed to set a start time for the transfer timeout.
*
* @return CHIP_ERROR Result of initialization and preparation of a TransferInit message. May also indicate if the
* TransferSession object is unable to handle this request.
*/
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData);
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout);

/**
* @brief
Expand All @@ -188,11 +190,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param xferControlOpts Indicates all supported control modes. Used to respond to a TransferInit message
* @param maxBlockSize The max Block size that this object supports.
* @param timeout The amount of time to wait for a response before considering the transfer failed
*
* @return CHIP_ERROR Result of initialization. May also indicate if the TransferSession object is unable to handle this
* request.
*/
CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize);
CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize,
System::Clock::Timeout timeout);

/**
* @brief
Expand Down
4 changes: 2 additions & 2 deletions src/protocols/bdx/TransferFacilitator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole rol
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize));
ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));

mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
Expand All @@ -109,7 +109,7 @@ CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role,
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData));
ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData, timeout));

mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
Expand Down
83 changes: 63 additions & 20 deletions src/protocols/bdx/tests/TestBdxTransferSession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ using namespace ::chip::bdx;
using namespace ::chip::Protocols;

namespace {
// Use this as a timestamp if not needing to test BDX timeouts.
constexpr System::Clock::Timestamp kNoAdvanceTime = System::Clock::kZero;

const TLV::Tag tlvStrTag = TLV::ContextTag(4);
const TLV::Tag tlvListTag = TLV::ProfileTag(7777, 8888);
Expand Down Expand Up @@ -84,7 +86,7 @@ CHIP_ERROR AttachHeaderAndSend(TransferSession::MessageTypeData typeData, chip::
chip::PayloadHeader payloadHeader;
payloadHeader.SetMessageType(typeData.ProtocolId, typeData.MessageType);

ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf)));
ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf), kNoAdvanceTime));
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -129,7 +131,7 @@ void VerifyStatusReport(nlTestSuite * inSuite, void * inContext, const System::P
void VerifyNoMoreOutput(nlTestSuite * inSuite, void * inContext, TransferSession & transferSession)
{
TransferSession::OutputEvent event;
transferSession.PollOutput(event);
transferSession.PollOutput(event, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, event.EventType == TransferSession::OutputEventType::kNone);
}

Expand All @@ -145,22 +147,22 @@ void SendAndVerifyTransferInit(nlTestSuite * inSuite, void * inContext, Transfer
MessageType expectedInitMsg = (initiatorRole == TransferRole::kSender) ? MessageType::SendInit : MessageType::ReceiveInit;

// Initializer responder to wait for transfer
err = responder.WaitForTransfer(responderRole, responderControlOpts, responderMaxBlock);
err = responder.WaitForTransfer(responderRole, responderControlOpts, responderMaxBlock, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
VerifyNoMoreOutput(inSuite, inContext, responder);

// Verify initiator outputs respective Init message (depending on role) after StartTransfer()
err = initiator.StartTransfer(initiatorRole, initData);
err = initiator.StartTransfer(initiatorRole, initData, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiator.PollOutput(outEvent);
initiator.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedInitMsg);
VerifyNoMoreOutput(inSuite, inContext, initiator);

// Verify that all parsed TransferInit fields match what was sent by the initiator
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), responder);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
responder.PollOutput(outEvent);
responder.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, responder);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInitReceived);
NL_TEST_ASSERT(inSuite, outEvent.transferInitData.TransferCtlFlags == initData.TransferCtlFlags);
Expand Down Expand Up @@ -213,7 +215,7 @@ void SendAndVerifyAcceptMsg(nlTestSuite * inSuite, void * inContext, TransferSes
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// Verify Sender emits ReceiveAccept message for sending
acceptSender.PollOutput(outEvent);
acceptSender.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, acceptSender);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedMsg);
Expand All @@ -225,7 +227,7 @@ void SendAndVerifyAcceptMsg(nlTestSuite * inSuite, void * inContext, TransferSes
// Verify received ReceiveAccept.
// Client may want to inspect TransferControl, MaxBlockSize, StartOffset, Length, and Metadata, and may choose to reject the
// Transfer at this point.
acceptReceiver.PollOutput(outEvent);
acceptReceiver.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, acceptReceiver);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kAcceptReceived);
NL_TEST_ASSERT(inSuite, outEvent.transferAcceptData.ControlMode == acceptData.ControlMode);
Expand Down Expand Up @@ -260,15 +262,15 @@ void SendAndVerifyQuery(nlTestSuite * inSuite, void * inContext, TransferSession
// Verify that querySender emits BlockQuery message
CHIP_ERROR err = querySender.PrepareBlockQuery();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
querySender.PollOutput(outEvent);
querySender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, MessageType::BlockQuery);
VerifyNoMoreOutput(inSuite, inContext, querySender);

// Pass BlockQuery to queryReceiver and verify queryReceiver emits QueryReceived event
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), queryReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
queryReceiver.PollOutput(outEvent);
queryReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kQueryReceived);
VerifyNoMoreOutput(inSuite, inContext, queryReceiver);
}
Expand Down Expand Up @@ -303,15 +305,15 @@ void SendAndVerifyArbitraryBlock(nlTestSuite * inSuite, void * inContext, Transf
// Provide Block data and verify sender emits Block message
err = sender.PrepareBlock(blockData);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
sender.PollOutput(outEvent);
sender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expected);
VerifyNoMoreOutput(inSuite, inContext, sender);

// Pass Block message to receiver and verify matching Block is received
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), receiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
receiver.PollOutput(outEvent);
receiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kBlockReceived);
NL_TEST_ASSERT(inSuite, outEvent.blockdata.Data != nullptr);
if (outEvent.EventType == TransferSession::OutputEventType::kBlockReceived && outEvent.blockdata.Data != nullptr)
Expand All @@ -333,15 +335,15 @@ void SendAndVerifyBlockAck(nlTestSuite * inSuite, void * inContext, TransferSess
// Verify PrepareBlockAck() outputs message to send
CHIP_ERROR err = ackSender.PrepareBlockAck();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
ackSender.PollOutput(outEvent);
ackSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedMsgType);
VerifyNoMoreOutput(inSuite, inContext, ackSender);

// Pass BlockAck to ackReceiver and verify it was received
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), ackReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
ackReceiver.PollOutput(outEvent);
ackReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == expectedEventType);
VerifyNoMoreOutput(inSuite, inContext, ackReceiver);
}
Expand Down Expand Up @@ -574,6 +576,46 @@ void TestBadAcceptMessageFields(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR);
}

// Test that a TransferSession will emit kTransferTimeout if the specified timeout is exceeded while waiting for a response.
void TestTimeout(nlTestSuite * inSuite, void * inContext)
{
CHIP_ERROR err = CHIP_NO_ERROR;
TransferSession initiator;
TransferSession::OutputEvent outEvent;

System::Clock::Timeout timeout = System::Clock::Milliseconds32(24);
System::Clock::Timestamp startTime = System::Clock::Milliseconds64(100);
System::Clock::Timestamp endTime = System::Clock::Milliseconds64(124);

// Initialize struct with arbitrary TransferInit parameters
TransferSession::TransferInitData initOptions;
initOptions.TransferCtlFlags = TransferControlFlags::kReceiverDrive;
initOptions.MaxBlockSize = 64;
initOptions.StartOffset = 0;
initOptions.Length = 0;
char testFileDes[9] = { "test.txt" }; // arbitrary file designator
initOptions.FileDesLength = static_cast<uint16_t>(strlen(testFileDes));
initOptions.FileDesignator = reinterpret_cast<uint8_t *>(testFileDes);
initOptions.Metadata = nullptr;
initOptions.MetadataLength = 0;

TransferRole role = TransferRole::kReceiver;

// Verify initiator outputs respective Init message (depending on role) after StartTransfer()
err = initiator.StartTransfer(role, initOptions, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// First PollOutput() should output the TransferInit message
initiator.PollOutput(outEvent, startTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
MessageType expectedInitMsg = (role == TransferRole::kSender) ? MessageType::SendInit : MessageType::ReceiveInit;
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedInitMsg);

// Second PollOutput() with no call to HandleMessageReceived() should result in a timeout.
initiator.PollOutput(outEvent, endTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout);
}

// Test that sending the same block twice (with same block counter) results in a StatusReport message with BadBlockCounter. Also
// test that receiving the StatusReport ends the transfer on the other node.
void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
Expand Down Expand Up @@ -633,7 +675,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Provide Block data and verify sender emits Block message
err = respondingSender.PrepareBlock(blockData);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
respondingSender.PollOutput(eventWithBlock);
respondingSender.PollOutput(eventWithBlock, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, eventWithBlock.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, eventWithBlock, MessageType::Block);
VerifyNoMoreOutput(inSuite, inContext, respondingSender);
Expand All @@ -643,7 +685,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Pass Block message to receiver and verify matching Block is received
err = AttachHeaderAndSend(eventWithBlock.msgTypeData, std::move(eventWithBlock.MsgData), initiatingReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kBlockReceived);
NL_TEST_ASSERT(inSuite, outEvent.blockdata.Data != nullptr);
VerifyNoMoreOutput(inSuite, inContext, initiatingReceiver);
Expand All @@ -653,7 +695,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Verify receiving same Block twice fails and results in StatusReport event, and then InternalError event
err = AttachHeaderAndSend(eventWithBlock.msgTypeData, std::move(blockCopy), initiatingReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
System::PacketBufferHandle statusReportMsg = outEvent.MsgData.Retain();
TransferSession::MessageTypeData statusReportMsgTypeData = outEvent.msgTypeData;
Expand All @@ -662,21 +704,21 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// All subsequent PollOutput() calls should return kInternalError
for (int i = 0; i < 5; ++i)
{
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInternalError);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);
}

err = AttachHeaderAndSend(statusReportMsgTypeData, std::move(statusReportMsg), respondingSender);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
respondingSender.PollOutput(outEvent);
respondingSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kStatusReceived);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);

// All subsequent PollOutput() calls should return kInternalError
for (int i = 0; i < 5; ++i)
{
respondingSender.PollOutput(outEvent);
respondingSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInternalError);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);
}
Expand All @@ -693,6 +735,7 @@ static const nlTest sTests[] =
NL_TEST_DEF("TestInitiatingReceiverReceiverDrive", TestInitiatingReceiverReceiverDrive),
NL_TEST_DEF("TestInitiatingSenderSenderDrive", TestInitiatingSenderSenderDrive),
NL_TEST_DEF("TestBadAcceptMessageFields", TestBadAcceptMessageFields),
NL_TEST_DEF("TestTimeout", TestTimeout),
NL_TEST_DEF("TestDuplicateBlockError", TestDuplicateBlockError),
NL_TEST_SENTINEL()
};
Expand Down

0 comments on commit 3a9868e

Please sign in to comment.