Fix/log message received threaded#3
Closed
kurobon-jp wants to merge 4 commits into
Closed
Conversation
🐙 Octopus Review — PR #3SummaryThis PR fixes two issues with Score
Risk Assessment
Positive Highlights
Important Files Changed
Diagramgraph TD
LogThread["Background Thread"]
MainThread["Unity Main Thread"]
LogThread -- "logMessageReceivedThreaded" --> Handler["OnLogMessageReceived()"]
Handler --> GuardCheck{"IsShowErrorIndicator?"}
GuardCheck -- "false" --> Return["return (safe)"]
GuardCheck -- "true" --> TypeCheck{"Error/Exception/Assert?"}
TypeCheck -- "false or _isBlinking" --> Return2["return (safe)"]
TypeCheck -- "true" --> SetError["HasError = true"]
SetError --> StopCoroutines["StopAllCoroutines()"]
StopCoroutines --> StartCoroutine["StartCoroutine(Blink())"]
StartCoroutine --> MainThreadRequired["⚠️ Requires Main Thread"]
classDef changed fill:#fbbf24,stroke:#92400e,color:#000
classDef warning fill:#f87171,stroke:#991b1b,color:#000
classDef safe fill:#4ade80,stroke:#166534,color:#000
classDef unchanged fill:#374151,stroke:#6b7280,color:#e5e7eb
class Handler,GuardCheck changed
class StopCoroutines,StartCoroutine,MainThreadRequired warning
class Return,Return2 safe
class LogThread,MainThread,TypeCheck,SetError unchanged
Last reviewed commit: 8c738bd Checklist
|
![octopus-review[bot]](https://avatars.githubusercontent.com/in/2917745?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 files reviewed, 2 findings | View scores & details
🔴 Findings that could not be mapped to diff lines:
| Severity | File | Title | Description |
|---|---|---|---|
| 🔴 | Assets/Runtime/Scripts/Log/ErrorIndicator.cs:L42 |
MonoBehaviour Coroutine methods called from background thread | OnLogMessageReceived is now registered with Application.logMessageReceivedThreaded, which can be invoked from any thread… |
🟡 Additional findings
| Severity | File | Title | Description |
|---|---|---|---|
| 🟡 | Assets/Runtime/Scripts/Log/ErrorIndicator.cs:L23 |
IsShowErrorIndicator guard removed from subscription site — always subscribes regardless of setting | The original code only subscribed to the log event when IsShowErrorIndicator was true. The new code unconditionally subs… |
Reviewed by Octopus Review, an AI-powered PR review tool.
🐙 Octopus Review — PR #3SummaryこのPRは主に3つの変更を含みます。① Score
Risk Assessment
Positive Highlights
Important Files Changed
DiagramsequenceDiagram
participant Worker as Worker Thread
participant Handler as OnLogMessageReceived
participant Unity as Unity Main Thread
Worker->>Handler: Application.logMessageReceivedThreaded
activate Handler
Handler->>Handler: IsShowErrorIndicator check
Handler->>Handler: type / _isBlinking check
Handler->>Unity: HasError = true
Handler->>Unity: StopAllCoroutines() ← 🔴 NOT thread-safe
Handler->>Unity: StartCoroutine(Blink()) ← 🔴 NOT thread-safe
deactivate Handler
note over Unity: Unity API must be called\nfrom main thread only
Last reviewed commit: e9c5114 Checklist
Reviewed by Octopus |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| Application.logMessageReceivedThreaded += OnLogMessageReceived; | ||
| Clear(); |
There was a problem hiding this comment.
🟡 Unconditional subscription when ErrorIndicator is disabled by settings
The old code only subscribed to logMessageReceived when IsShowErrorIndicator was true, avoiding any overhead when the feature is disabled. The new code always subscribes to logMessageReceivedThreaded, and only guards inside the callback. While the overhead per call is small, this now means every log message (including from high-frequency systems) invokes the callback and SDSettings lookup even when the indicator is permanently disabled. Consider retaining the subscription guard, or at minimum caching IsShowErrorIndicator at subscribe time.
Suggested change
| Clear(); | |
| protected override void OnEnable() | |
| { | |
| // Still subscribe unconditionally if you want runtime toggling, | |
| // but cache the setting to avoid per-call SDSettings lookup: | |
| // _isIndicatorEnabled = SDSettings.Instance.IsShowErrorIndicator; | |
| Application.logMessageReceivedThreaded += OnLogMessageReceived; | |
| Clear(); | |
| } |
AI Fix Prompt
Fix the following Medium (Performance) issue in `Assets/Runtime/Scripts/Log/ErrorIndicator.cs` at line 23:
Problem: The old code only subscribed to `logMessageReceived` when `IsShowErrorIndicator` was true, avoiding any overhead when the feature is disabled. The new code always subscribes to `logMessageReceivedThreaded`, and only guards inside the callback. While the overhead per call is small, this now means every log message (including from high-frequency systems) invokes the callback and SDSettings lookup even when the indicator is permanently disabled. Consider retaining the subscription guard, or at minimum caching `IsShowErrorIndicator` at subscribe time.
Suggested fix:
protected override void OnEnable()
{
// Still subscribe unconditionally if you want runtime toggling,
// but cache the setting to avoid per-call SDSettings lookup:
// _isIndicatorEnabled = SDSettings.Instance.IsShowErrorIndicator;
Application.logMessageReceivedThreaded += OnLogMessageReceived;
Clear();
}
| Unity特有の問題を優先してください。 | ||
|
|
||
| 明確な問題のみ指摘してください。 | ||
| 推測ベースの指摘は避けてください。 No newline at end of file |
There was a problem hiding this comment.
💡 Workflow file missing newline at end of file
The YAML file is missing a trailing newline (\ No newline at end of file). While harmless, POSIX-compliant text files should end with a newline, and some YAML linters will warn about this.
Suggested change
| 推測ベースの指摘は避けてください。 | |
| Unity特有の問題を優先してください。 | |
| 明確な問題のみ指摘してください。 | |
| 推測ベースの指摘は避けてください。 | |
AI Fix Prompt
Fix the following Nit (Style) issue in `.github/workflows/octopus.yaml` at line 73:
Problem: The YAML file is missing a trailing newline (`\ No newline at end of file`). While harmless, POSIX-compliant text files should end with a newline, and some YAML linters will warn about this.
Suggested fix:
Unity特有の問題を優先してください。
明確な問題のみ指摘してください。
推測ベースの指摘は避けてください。
There was a problem hiding this comment.
3 files reviewed, 0 findings | View scores & details
Reviewed by Octopus Review, an AI-powered PR review tool.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.