Skip to content

improve error logging with file paths and context#352

Closed
Sahil-u07 wants to merge 2 commits intoControlCore-Project:devfrom
Sahil-u07:improve/cpp-error-logging
Closed

improve error logging with file paths and context#352
Sahil-u07 wants to merge 2 commits intoControlCore-Project:devfrom
Sahil-u07:improve/cpp-error-logging

Conversation

@Sahil-u07
Copy link
Copy Markdown
Contributor

Enhanced error reporting in concore.hpp so I/O failures include contextual details (file path/port/name and retry count) and are written to standard error. This makes debugging read/write failures much clearer while preserving existing behavior.

Copilot AI review requested due to automatic review settings February 16, 2026 00:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Enhances I/O failure logging in the Concore C++ implementation (concore.hpp) by emitting contextual read/write error messages (including file paths/ports/names and retry count) to standard error to aid debugging.

Changes:

  • Replaced generic "Read error" / "skipping ..." stdout messages with contextual stderr messages for file-based reads/writes.
  • Added contextual stderr messages for shared-memory read/write failures, including retry count for reads.
  • Removed a stale comment about observed retry counts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

concore.hpp Outdated
//observed retry count in C++ from various tests is approx 80.
catch(...){
cout<<"Read error";
cerr << "Error reading " << inpath << port << "/" << name << " (retry " << retrycount << ")" << endl;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

This log line is inside a retry loop; using endl forces a flush on every iteration and can significantly slow down retries / spam stderr. Prefer writing a newline without flushing (e.g., '\n') and consider throttling the log (e.g., only every N retries) to avoid unbounded output if the read keeps failing.

Suggested change
cerr << "Error reading " << inpath << port << "/" << name << " (retry " << retrycount << ")" << endl;
if (retrycount % 100 == 0) {
cerr << "Error reading " << inpath << port << "/" << name << " (retry " << retrycount << ")" << '\n';
}

Copilot uses AI. Check for mistakes.
concore.hpp Outdated
//observed retry count in C++ from various tests is approx 80.
catch(...){
std::cout << "Read error" << std::endl;
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retrycount << ")" << endl;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

This retry-loop log also uses endl (flush) on every iteration; that can materially degrade performance and overwhelm logs if shared memory stays unavailable. Prefer a non-flushing newline and consider rate-limiting the message (e.g., every N retries).

Suggested change
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retrycount << ")" << endl;
if (retrycount % 100 == 0) {
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retrycount << ")" << '\n';
}

Copilot uses AI. Check for mistakes.
@Sahil-u07 Sahil-u07 force-pushed the improve/cpp-error-logging branch from fae48c6 to de31ee8 Compare February 16, 2026 00:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (2)

concore.hpp:367

  • If the input stays empty after MAX_RETRY attempts, the loop exits with ins == "" and the code later calls parser(ins) / accesses inval[0], which will throw or crash (empty string indexing / out-of-bounds). Add an explicit post-loop fallback (e.g., set ins = initstr or return an empty/default vector) before parsing.
                cerr << "Error reading " << inpath << port << "/" << name << " (retry " << retry << ")" << endl;
            }
            retry++;
        }
        s += ins;

concore.hpp:426

  • If shared memory remains empty through MAX_RETRY, the loop exits with ins == "" and the code later calls parser(ins) / accesses inval[0], which will throw or crash. Add a post-loop guard/fallback before parsing when ins is still empty.
                cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retry << ")" << endl;
            }
            retry++;
        }
        s += ins;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cout<<"Read error";
cerr << "Error reading " << inpath << port << "/" << name << " (retry " << retry << ")" << endl;
}
retry++;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

retrycount is a public field used elsewhere in the repo (e.g., scripts print concore.retrycount), but it is no longer incremented during retry attempts here. If the retry count is still intended to be observable, increment retrycount on each failed retry (or remove the field and update callers/docs).

Suggested change
retry++;
retry++;
retrycount++;

Copilot uses AI. Check for mistakes.
//observed retry count in C++ from various tests is approx 80.
catch(...){
std::cout << "Read error" << std::endl;
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retry << ")" << endl;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

retrycount is a public field used elsewhere in the repo (callers print concore.retrycount), but it is no longer incremented during retry attempts in the shared-memory read loop. If this metric is still relied upon, increment it on each failed retry (or remove the field and update callers/docs).

Suggested change
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retry << ")" << endl;
cerr << "Error reading from shared memory port " << port << " name " << name << " (retry " << retry << ")" << endl;
retrycount++;

Copilot uses AI. Check for mistakes.
Comment on lines 545 to +547
std::string result = outfile.str();
std::strncpy(sharedData_create, result.c_str(), 256 - 1);
simtime += delta;
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

std::strncpy(sharedData_create, result.c_str(), 256 - 1) does not guarantee null-termination when the source is >= 255 bytes, which can leave stale bytes in shared memory and cause readers to see corrupted/concatenated messages. Explicitly null-terminate the buffer (and consider clearing it before copy) after the strncpy.

Copilot uses AI. Check for mistakes.
Comment on lines 498 to 500
catch(...){
cout<<"skipping +"<<outpath<<port<<" /"<<name;
cerr << "Error writing to " << outpath << port << "/" << name << endl;
}
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The new error log in this catch(...) still drops the underlying failure reason. Prefer catching const std::exception& where possible and including e.what() (and/or errno/std::strerror(errno) for I/O failures) so the log is actionable beyond just the path.

Copilot uses AI. Check for mistakes.
@pradeeban
Copy link
Copy Markdown
Member

Removing retrycount++; makes the code hard to read and understand by humans.

@pradeeban pradeeban closed this Feb 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants