MEDIUM: Inconsistent Error Handling Throughout Codebase

[juliensimon]

Issue Description

Maintainability issue: Codebase uses inconsistent error handling strategies including mix of exceptions, return codes, and silent failures, making debugging difficult and system behavior unpredictable.

Location

Files: Multiple files throughout the codebase
Affected Areas:

• common/memory_utils.cpp - Mix of return codes and exceptions
• common/aligned_buffer.cpp - Exception-based error handling
• common/platform_factory.cpp - Exception throwing
• common/argument_parser.cpp - Mixed error handling approaches
• Various other modules with inconsistent patterns

Maintainability Impact

• Severity: MEDIUM (P2)
• Impact: Difficult debugging, unpredictable error behavior
• Affected Operations: Error detection, logging, recovery
• Critical for: Production stability and debugging

Technical Details

Current Inconsistent Patterns

Pattern 1: Exception-based (AlignedBuffer)

AlignedBuffer::AlignedBuffer(size_t size, size_t alignment) {
    if (size == 0) {
        throw MemoryError("Buffer size cannot be zero");  // Exception
    }
    if (!is_power_of_two(alignment)) {
        throw MemoryError("Alignment must be a power of 2");  // Exception
    }
}

Pattern 2: Return Code-based (Memory Utils)

bool validate_buffer_range(size_t start_offset, size_t end_offset, 
                          size_t buffer_size, size_t min_size) {
    if (start_offset >= end_offset) {
        return false;  // Return code - no error information
    }
    // More return false cases...
}

Pattern 3: Silent Failures (Platform Factory)

size_t calculate_buffer_size(size_t total_size, size_t num_buffers, 
                            size_t cache_line_size) {
    if (total_size == 0 || num_buffers == 0) {
        return 0;  // Silent failure - no error indication
    }
}

Problems with Current Approach

1. Mixed error handling strategies make error flows unpredictable
2. Loss of error context with boolean return codes
3. Silent failures hide important error conditions
4. Inconsistent error logging and reporting
5. Difficult debugging due to unclear error propagation
6. No standardized error recovery mechanisms

Expected Behavior

• Consistent error handling strategy across the entire codebase
• Rich error information with context and stack traces
• Standardized error logging and reporting
• Clear error recovery and handling patterns
• Predictable error behavior for debugging

Actual Behavior

• Three different error handling patterns used inconsistently
• Some errors throw exceptions, others return false, others fail silently
• Error context is often lost or incomplete
• Debugging is difficult due to unclear error sources
• No consistent error recovery strategy

Examples of Inconsistencies

Memory Allocation Errors

// File 1: Exception approach
if (allocation_failed) {
    throw MemoryError("Failed to allocate memory");
}

// File 2: Return code approach  
if (allocation_failed) {
    return nullptr;  // No error context
}

// File 3: Silent failure approach
if (allocation_failed) {
    return 0;  // Caller may not realize it's an error
}

Input Validation Errors

// Inconsistent validation patterns
bool validate_input_v1(const Config& config) {
    return config.size > 0;  // No error details
}

void validate_input_v2(const Config& config) {
    if (config.size <= 0) {
        throw std::invalid_argument("Size must be positive");
    }
}

int validate_input_v3(const Config& config) {
    if (config.size <= 0) return -1;  // Magic number
    return 0;
}

Suggested Solution

// Unified error handling system

// 1. Define comprehensive error hierarchy
class BenchmarkError : public std::runtime_error {
public:
    enum class Category {
        MEMORY_ERROR,
        VALIDATION_ERROR, 
        PLATFORM_ERROR,
        CONFIGURATION_ERROR,
        IO_ERROR
    };
    
    BenchmarkError(Category cat, const std::string& message, 
                   const std::string& context = "")
        : std::runtime_error(message)
        , category_(cat)
        , context_(context)
        , timestamp_(std::chrono::system_clock::now()) {
    }
    
    Category category() const { return category_; }
    const std::string& context() const { return context_; }
    
private:
    Category category_;
    std::string context_;
    std::chrono::system_clock::time_point timestamp_;
};

// 2. Specific error types
class MemoryError : public BenchmarkError {
public:
    MemoryError(const std::string& message, const std::string& context = "")
        : BenchmarkError(Category::MEMORY_ERROR, message, context) {}
};

class ValidationError : public BenchmarkError {
public:
    ValidationError(const std::string& message, const std::string& context = "")
        : BenchmarkError(Category::VALIDATION_ERROR, message, context) {}
};

// 3. Result type for operations that can fail
template<typename T>
class Result {
public:
    static Result success(T value) {
        return Result(std::move(value));
    }
    
    static Result error(const std::string& message) {
        return Result(message);
    }
    
    bool is_success() const { return has_value_; }
    bool is_error() const { return !has_value_; }
    
    const T& value() const {
        if (!has_value_) {
            throw std::runtime_error("Attempted to get value from error result: " + error_message_);
        }
        return value_;
    }
    
    const std::string& error() const {
        if (has_value_) {
            throw std::runtime_error("Attempted to get error from success result");
        }
        return error_message_;
    }
    
private:
    explicit Result(T value) : value_(std::move(value)), has_value_(true) {}
    explicit Result(std::string error) : error_message_(std::move(error)), has_value_(false) {}
    
    T value_;
    std::string error_message_;
    bool has_value_;
};

// 4. Consistent error handling patterns
class UnifiedMemoryUtils {
public:
    // Option 1: Exception-based for critical errors
    void critical_operation() {
        if (critical_failure_condition) {
            throw MemoryError("Critical memory allocation failed", 
                            "UnifiedMemoryUtils::critical_operation");
        }
    }
    
    // Option 2: Result-based for recoverable errors
    Result<size_t> recoverable_operation(size_t input) {
        if (input == 0) {
            return Result<size_t>::error("Input cannot be zero");
        }
        
        size_t result = process_input(input);
        if (result == 0) {
            return Result<size_t>::error("Processing failed");
        }
        
        return Result<size_t>::success(result);
    }
};

// 5. Centralized error logging
class ErrorLogger {
public:
    static void log_error(const BenchmarkError& error) {
        std::cerr << "[ERROR] " << category_to_string(error.category())
                  << ": " << error.what();
        if (!error.context().empty()) {
            std::cerr << " (Context: " << error.context() << ")";
        }
        std::cerr << std::endl;
    }
    
private:
    static std::string category_to_string(BenchmarkError::Category cat) {
        switch (cat) {
            case BenchmarkError::Category::MEMORY_ERROR: return "MEMORY";
            case BenchmarkError::Category::VALIDATION_ERROR: return "VALIDATION";
            case BenchmarkError::Category::PLATFORM_ERROR: return "PLATFORM";
            case BenchmarkError::Category::CONFIGURATION_ERROR: return "CONFIG";
            case BenchmarkError::Category::IO_ERROR: return "IO";
            default: return "UNKNOWN";
        }
    }
};

Acceptance Criteria

• Define unified error handling strategy for the entire codebase
• Implement consistent error hierarchy with rich context
• Replace all boolean return codes with Result or exceptions
• Eliminate silent failures throughout codebase
• Add centralized error logging and reporting
• Document error handling guidelines for contributors
• Add unit tests for error handling scenarios
• Validate error handling with comprehensive test coverage

Implementation Plan

1. Phase 1: Define error hierarchy and Result type
2. Phase 2: Migrate critical paths (memory allocation, validation)
3. Phase 3: Update remaining modules for consistency
4. Phase 4: Add comprehensive error handling tests
5. Phase 5: Document error handling guidelines

Testing Requirements

• Unit tests for each error type and scenario
• Integration tests for error propagation
• Error handling performance impact analysis
• Validation that no errors are silently ignored
• Test error recovery and graceful degradation

Migration Strategy

// Before (inconsistent):
bool old_function(int input) {
    if (input < 0) return false;  // Lost context
    return process(input);
}

// After (consistent):
Result<ProcessedData> new_function(int input) {
    if (input < 0) {
        return Result<ProcessedData>::error(
            "Input must be non-negative, got: " + std::to_string(input));
    }
    
    try {
        auto result = process(input);
        return Result<ProcessedData>::success(result);
    } catch (const std::exception& e) {
        return Result<ProcessedData>::error(
            "Processing failed: " + std::string(e.what()));
    }
}

Error Handling Guidelines

1. Use exceptions for:

   • Critical system failures (out of memory, hardware errors)
   • Programming errors (invalid arguments to internal functions)
   • Unrecoverable errors that should terminate execution

2. Use Result for:

   • User input validation errors
   • Recoverable operational errors
   • Functions where failure is part of normal operation

3. Never use:

   • Silent failures (returning 0, nullptr without indication)
   • Boolean returns without error context
   • Magic number return codes

Expected Benefits

• Improved debugging: Clear error sources and context
• Better user experience: Meaningful error messages
• Easier maintenance: Consistent error handling patterns
• Reduced bugs: Elimination of silent failures
• Enhanced testing: Comprehensive error scenario coverage

References

• C++ Core Guidelines: Error Handling
• Effective C++: Exception Safety
• Rust Result Pattern Documentation

Priority: MEDIUM - Important for maintainability and debugging
Estimated Effort: 5-7 days
Code Review Required: Yes