diff --git a/n_request.c b/n_request.c index 10e790be..60ee4bae 100644 --- a/n_request.c +++ b/n_request.c @@ -491,6 +491,12 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard) // Calculate the transaction timeout based on the parameters in the request. const uint32_t transactionTimeoutMs = _noteTransaction_calculateTimeoutMs(req, reqFound); + // Take the lock on the Notecard. This is required to ensure that we don't + // have multiple threads trying to access the Notecard at the same time. + if (lockNotecard) { + _LockNote(); + } + #ifndef NOTE_C_LOW_MEM /* * Add a CRC value, so the request may be retried if it is received @@ -510,9 +516,10 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard) * 1 0 1 * 1 1 1 (UB) */ + const uint16_t transactionSeqNo = seqNo; bool crcAddedToRequest = false; if (reqFound) { - char *newJson = _crcAdd(json, seqNo); + char *newJson = _crcAdd(json, transactionSeqNo); if (newJson != NULL) { _Free(json); json = newJson; @@ -521,12 +528,6 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard) } #endif // !NOTE_C_LOW_MEM - // Take the lock on the Notecard. This is required to ensure that we don't - // have multiple threads trying to access the Notecard at the same time. - if (lockNotecard) { - _LockNote(); - } - // If a reset of the I/O interface is required for any reason, do it now. if (resetRequired) { NOTE_C_LOG_DEBUG("Resetting Notecard I/O Interface..."); @@ -627,7 +628,7 @@ J *_noteTransactionShouldLock(J *req, bool lockNotecard) // If we sent a CRC in the request, examine the response JSON to see if // it has a CRC error. Note that the CRC is stripped from the // rspJsonStr as a side-effect of this method. - if (crcAddedToRequest && _crcError(rspJsonStr, seqNo)) { + if (crcAddedToRequest && _crcError(rspJsonStr, transactionSeqNo)) { _Free(rspJsonStr); errStr = ERRSTR("CRC error {io}", c_iobad); NOTE_C_LOG_WARN(ERRSTR("retrying... CRC error", c_iobad)); diff --git a/test/src/NoteTransaction_test.cpp b/test/src/NoteTransaction_test.cpp index ee64e1d3..5c91ce20 100644 --- a/test/src/NoteTransaction_test.cpp +++ b/test/src/NoteTransaction_test.cpp @@ -166,16 +166,39 @@ class MultipleHeartbeatsThenValid }; int MultipleHeartbeatsThenValid::call_count = 0; +bool noteTransactionTestNotecardLocked = false; +bool crcAddCalledWhileNotecardLocked = false; + +void noteTransactionTestLockNote(void) +{ + noteTransactionTestNotecardLocked = true; +} + +void noteTransactionTestUnlockNote(void) +{ + noteTransactionTestNotecardLocked = false; +} + char * _crcAdd_customFake(char *json, uint16_t seqno) { // Custom fake implementation for _crcAdd + (void)seqno; return strdup(json); } +char * _crcAdd_lockCheckCustomFake(char *json, uint16_t seqno) +{ + crcAddCalledWhileNotecardLocked = noteTransactionTestNotecardLocked; + return _crcAdd_customFake(json, seqno); +} + SCENARIO("NoteTransaction") { NoteSetFnDefault(malloc, free, NULL, NULL); + NoteSetFnNoteMutex(NULL, NULL); _crcAdd_fake.custom_fake = _crcAdd_customFake; + noteTransactionTestNotecardLocked = false; + crcAddCalledWhileNotecardLocked = false; // NoteReset's mock should succeed unless the test explicitly instructs // it to fail. @@ -449,6 +472,26 @@ SCENARIO("NoteTransaction") } #ifndef NOTE_C_LOW_MEM + SECTION("CRC is added while the Notecard lock is held") { + J *req = NoteNewRequest("note.add"); + REQUIRE(req != NULL); + _noteJSONTransaction_fake.custom_fake = _noteJSONTransactionValid; + _crcAdd_fake.custom_fake = _crcAdd_lockCheckCustomFake; + NoteSetFnNoteMutex(noteTransactionTestLockNote, noteTransactionTestUnlockNote); + + J *resp = NoteTransaction(req); + + CHECK(_crcAdd_fake.call_count == 1); + CHECK(crcAddCalledWhileNotecardLocked); + CHECK(resp != NULL); + CHECK(!NoteResponseError(resp)); + CHECK(!noteTransactionTestNotecardLocked); + + NoteSetFnNoteMutex(NULL, NULL); + JDelete(req); + JDelete(resp); + } + SECTION("Bad CRC") { J *req = NoteNewRequest("note.add"); REQUIRE(req != NULL);