Skip to content

[AI Task] Fault NfcTag transceive/read NDEF tasks on native error and remove double dictionary lookup - #7774

Open
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7675
Open

[AI Task] Fault NfcTag transceive/read NDEF tasks on native error and remove double dictionary lookup#7774
JoonghyunCho wants to merge 2 commits into
mainfrom
ai-task/issue-7675

Conversation

@JoonghyunCho

Copy link
Copy Markdown
Member

Summary

Fixes a silent-failure bug in the NFC tag async callbacks: when the native transceive or NDEF-read completed with an error, the TaskCompletionSource was removed from the tracking dictionary without ever being completed, so awaiting TransceiveAsync() / ReadNdefMessageAsync() hung forever and the captured async state machine, TCS, and buffers leaked. The callbacks now fault the task with InvalidOperationException (consistent with the documented exception contract and with the write/format paths). The redundant ContainsKey + indexer double lookup is also collapsed into a single TryGetValue.

Changes

  • src/Tizen.Network.Nfc/Tizen.Network.Nfc/NfcTag.cs
    • TransceiveCompletedCallback: ContainsKey + indexer → TryGetValue; on non-None result, TrySetException(new InvalidOperationException(((NfcError)result).ToString())) so the awaiting caller faults instead of hanging.
    • ReadNdefCallback: same treatment; the callback's bool return value on error is unchanged (false).
    • Success paths, dictionary removal, and VoidCallback (already completes its TCS on every result) are untouched.

Mode

Refactoring

Verification

  • Build: passed (dotnet build src/Tizen.Network.Nfc/Tizen.Network.Nfc.csproj — 0 errors, 0 warnings)
  • Tests: N/A (no unit test project covers this module; change is limited to private native-callback error paths)
  • Benchmark: skipped (sdb error: no device connected)

API Compatibility

  • Public API signatures: unchanged (both modified callbacks are private).
  • Behavior: on native failure the returned Task now faults, matching the documented InvalidOperationException contract, instead of never completing; success path is byte-for-byte identical.

Fixes #7675

@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions github-actions Bot added the API15 label Jul 26, 2026
@JoonghyunCho

Copy link
Copy Markdown
Member Author

⚠️ Benchmark skipped: No sdb device is connected in the automation environment (sdb devices returned an empty list), so the on-device benchmark could not be run. Manual benchmark verification is required. Note: this change only affects native-callback error paths (previously the task never completed); the success path is unchanged except for one fewer dictionary hash lookup per completion.

}
else
{
taskSource.TrySetException(new InvalidOperationException(((NfcError)result).ToString()));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
🟡 Suggestion: Faulting with a hardcoded InvalidOperationException bypasses the NfcErrorFactory mapping used by every synchronous error path in this file (NotSupportedError → NotSupportedException, InvalidParameterError → ArgumentException, both documented on TransceiveAsync), and drops the Log.Error parity of the other error branches.

Suggested change
taskSource.TrySetException(new InvalidOperationException(((NfcError)result).ToString()));
Log.Error(Globals.LogTag, $"Failed to transceive data, Error - {(NfcError)result}");
try
{
NfcErrorFactory.ThrowNfcException(result);
}
catch (Exception e)
{
taskSource.TrySetException(e);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
Addressed in efeb1a1 — the transceive error path now logs the failure and routes the native result through NfcErrorFactory.ThrowNfcException, so the faulted exception type matches the documented contract.

}
else
{
taskSource.TrySetException(new InvalidOperationException(((NfcError)result).ToString()));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
🟡 Suggestion: Same as TransceiveCompletedCallback — routing the error through NfcErrorFactory keeps the faulted exception type consistent with the documented contract and restores Log.Error parity.

Suggested change
taskSource.TrySetException(new InvalidOperationException(((NfcError)result).ToString()));
Log.Error(Globals.LogTag, $"Failed to read ndef message, Error - {(NfcError)result}");
try
{
NfcErrorFactory.ThrowNfcException(result);
}
catch (Exception e)
{
taskSource.TrySetException(e);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [AI Review]
Addressed in efeb1a1 — the read-NDEF error path now logs the failure and routes the native result through NfcErrorFactory.ThrowNfcException, matching TransceiveCompletedCallback.

@JoonghyunCho

Copy link
Copy Markdown
Member Author

🤖 [AI Review]
Left 2 inline comments (🔴 0, 🟡 2). Both concern the new error-fault paths: routing the native error through NfcErrorFactory keeps the exception type consistent with the documented contract, and restores Log.Error parity. The hang fix itself and the TryGetValue cleanup look correct.


Automated review by AI assistant

Route native errors through NfcErrorFactory in TransceiveCompletedCallback and ReadNdefCallback to keep faulted exception types consistent with the documented contract and restore Log.Error parity

Applied-AI-Comments: 3653647061,3653647146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant