263 lines
8.5 KiB
Markdown
263 lines
8.5 KiB
Markdown
# Code Improvement Analysis: EpsonPrintService & Inspectron.Epson
|
|
|
|
## Executive Summary
|
|
|
|
Analysis of both projects revealed **50+ distinct issues** across security, reliability, performance, and maintainability. The most critical issues involve resource management, thread safety, and missing cancellation support.
|
|
|
|
---
|
|
|
|
## Critical Issues (Fix Immediately)
|
|
|
|
### 1. Security: Plaintext API Key
|
|
**File:** `EpsonPrintService/config.json:4`
|
|
```json
|
|
"ApiKey": "Pd8X/VtXLWgl5Djy4ksjdHC3xboeoh5Jig3Qo4277GQ="
|
|
```
|
|
- API key stored in plaintext JSON file
|
|
- **Fix:** Move to environment variables or secure configuration provider
|
|
|
|
### 2. HttpClient Anti-Pattern (Socket Exhaustion Risk)
|
|
**File:** `EpsonPrintService/HeartbeatBackgroundTask.cs:24`
|
|
```csharp
|
|
_httpClient = new HttpClient(); // Created per instance
|
|
```
|
|
**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:68`
|
|
```csharp
|
|
using var httpClient = new HttpClient(); // Created per request
|
|
```
|
|
- **Fix:** Inject singleton HttpClient or use IHttpClientFactory
|
|
|
|
### 3. Missing Graceful Shutdown
|
|
**File:** `EpsonPrintService/Program.cs:51-82`
|
|
- `PrintLoop` and `PrintServer` never call `StopAsync()` on exit
|
|
- Queued print jobs are lost on Ctrl+C
|
|
- **Fix:** Store references and call shutdown methods before exiting
|
|
|
|
### 4. NullReferenceException Risk
|
|
**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:35-44`
|
|
```csharp
|
|
var printerId = await epsonPrinter.GetPrinterIdAsync();
|
|
var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...);
|
|
```
|
|
- `GetPrinterIdAsync()` can return null but `.Value` is called without check
|
|
- **Fix:** Add null check before accessing `.Value`
|
|
|
|
### 5. Thread Safety: Race Condition in PrintServer
|
|
**File:** `Inspectron.Epson/Queue/PrintServer.cs:19-23`
|
|
```csharp
|
|
if (_printerQueues.ContainsKey(printerIp))
|
|
return;
|
|
var queue = new PrinterQueue(...);
|
|
_printerQueues[printerIp] = queue;
|
|
```
|
|
- TOCTOU bug: two threads could create duplicate queues
|
|
- **Fix:** Use `ConcurrentDictionary.GetOrAdd()` or `TryAdd()`
|
|
|
|
---
|
|
|
|
## High Priority Issues
|
|
|
|
### 6. Missing CancellationToken Support
|
|
**File:** `Inspectron.Epson/EpsonPrinter.cs` - Lines 52, 96, 129, 205, 335, 408, etc.
|
|
- Core async methods don't accept CancellationToken
|
|
- Callers cannot cancel long-running operations
|
|
- **Fix:** Add optional `CancellationToken cancellationToken = default` to all async methods
|
|
|
|
### 7. Fire-and-Forget Tasks Without Exception Handling
|
|
**File:** `Inspectron.Epson/PrintServer/PrintLoop.cs:29`
|
|
```csharp
|
|
_= Task.Run(Loop); // No exception handling
|
|
```
|
|
**File:** `EpsonPrintService/Program.cs:71-72`
|
|
```csharp
|
|
_ = discoveryTask.StartAsync(cts.Token);
|
|
_ = heartbeatTask.StartAsync(cts.Token);
|
|
```
|
|
- Unobserved exceptions will terminate the process
|
|
- **Fix:** Use exception handler or track tasks for proper completion
|
|
|
|
### 8. Resource Leaks (IDisposable Not Implemented)
|
|
**File:** `EpsonPrintService/HeartbeatBackgroundTask.cs` - No IDisposable
|
|
- HttpClient never disposed
|
|
|
|
**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:13-14`
|
|
- SemaphoreSlim and CancellationTokenSource never disposed
|
|
|
|
**File:** `Inspectron.Epson/EpsonPrinter.cs:757-761`
|
|
```csharp
|
|
public async ValueTask DisposeAsync()
|
|
{
|
|
Disconnect();
|
|
await Task.CompletedTask; // Missing GC.SuppressFinalize
|
|
}
|
|
```
|
|
- **Fix:** Implement proper disposal pattern with try/finally
|
|
|
|
### 9. Synchronous Wait in Async Context
|
|
**File:** `Inspectron.Epson/Queue/PrintServer.cs:32`
|
|
```csharp
|
|
queue.StopAsync().Wait(); // Deadlock risk
|
|
```
|
|
- **Fix:** Use `await` or proper async pattern
|
|
|
|
### 10. Configuration Not Validated at Startup
|
|
**File:** `EpsonPrintService/Program.cs:20-21`
|
|
```csharp
|
|
var config = JsonSerializer.Deserialize<EpsonPrintServiceConfiguration>(...);
|
|
// No null check or validation
|
|
```
|
|
- **Fix:** Validate all required fields immediately after deserialization
|
|
|
|
---
|
|
|
|
## Medium Priority Issues
|
|
|
|
### 11. Console.WriteLine in Library Code
|
|
**File:** `Inspectron.Epson/Queue/PrintServer.cs:25, 34, 64`
|
|
```csharp
|
|
Console.WriteLine($"Printer {printerIp} registered");
|
|
```
|
|
- Library code should use ILogger only
|
|
- **Fix:** Replace with `_logger.LogInformation()`
|
|
|
|
### 12. Logger Factory Created Repeatedly
|
|
**File:** `EpsonPrintService/Program.cs:33-40`
|
|
- Creates new LoggerFactory for every ILogger injection
|
|
- **Fix:** Create factory once and bind as singleton
|
|
|
|
### 13. Magic Numbers Throughout
|
|
**File:** `Inspectron.Epson/EpsonPrinter.cs:263, 273, 282, 327, 463`
|
|
- Hard-coded ESC/POS bytes scattered in code
|
|
- **Fix:** Move to `EpsonCommands` static class
|
|
|
|
**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:87-88`
|
|
```csharp
|
|
if (job.RetryCount >= 3) // Magic number
|
|
```
|
|
- **Fix:** Extract to configurable constant
|
|
|
|
### 14. SOLID Violations
|
|
|
|
**SRP Violation - EpsonPrinter.cs (763 lines)**
|
|
- Handles: connection, status, image processing, text printing, cutting
|
|
- **Fix:** Split into smaller classes (ConnectionManager, StatusProvider, ImagePrinter)
|
|
|
|
**SRP Violation - EpsonPrintServiceConfiguration**
|
|
- Implements both `IPrinterConfigurationSource` AND `IAssignedPrinterRepository`
|
|
- **Fix:** Separate into two classes
|
|
|
|
### 15. Code Duplication
|
|
|
|
**Image Conversion:**
|
|
- `ConvertToRasterData()` and `ConvertToColumnFormat()` share 50%+ logic
|
|
- **Fix:** Extract common image processing pipeline
|
|
|
|
**Image Loading:**
|
|
- Same file loading pattern repeated 4 times in EpsonPrinter.cs
|
|
- **Fix:** Create private helper method
|
|
|
|
### 16. Performance Issue in Image Conversion
|
|
**File:** `Inspectron.Epson/EpsonImageConverter.cs:243-247`
|
|
```csharp
|
|
image.ProcessPixelRows(accessor => { ... }); // Called per pixel!
|
|
```
|
|
- O(n²) performance instead of O(n)
|
|
- **Fix:** Call ProcessPixelRows once and cache accessor
|
|
|
|
### 17. Missing Input Validation
|
|
- `ConnectAsync()` doesn't validate IP format
|
|
- `PrintTextAsync()` doesn't validate text encoding
|
|
- `FeedLinesAsync()` doesn't validate line count > 0
|
|
- **Fix:** Add guard clauses with ArgumentException
|
|
|
|
### 18. Static State Thread Safety
|
|
**File:** `Inspectron.Epson/EpsonPrinter.cs:38-43`
|
|
```csharp
|
|
private static bool _initialized;
|
|
private static void RegisterCodepages()
|
|
{
|
|
if (_initialized) return; // No lock!
|
|
Encoding.RegisterProvider(...);
|
|
_initialized = true;
|
|
}
|
|
```
|
|
- **Fix:** Use lock or `Interlocked.CompareExchange()`
|
|
|
|
---
|
|
|
|
## Low Priority Issues
|
|
|
|
### 19. Commented-Out Code
|
|
- `EpsonPrintService/Program.cs:27-28` - Old factory bindings
|
|
- `Inspectron.Epson/PrintServer/Printers/TM-T30III.cs:25` - Dead code
|
|
- **Fix:** Remove or track in issue tracker
|
|
|
|
### 20. TODO Comments Left in Code
|
|
**File:** `Inspectron.Epson/Queue/PrintJob.cs:5`
|
|
```csharp
|
|
// todo: change to ip address
|
|
public string IP { get; set; }
|
|
```
|
|
- **Fix:** Resolve or create proper issue
|
|
|
|
### 21. Unused Serilog Dependencies
|
|
**File:** `EpsonPrintService/EpsonPrintService.csproj:14-16`
|
|
- Serilog packages referenced but not used
|
|
- **Fix:** Remove unused packages
|
|
|
|
### 22. Inconsistent Logging
|
|
- String interpolation used instead of structured logging placeholders
|
|
- Log levels inconsistent (LogWarning for config errors that should fail startup)
|
|
- **Fix:** Use `LogWarning("Printer {PrinterId} not found", printerIp)` pattern
|
|
|
|
### 23. API Design Inconsistencies
|
|
- `PrintTextAsync()` vs `PrintTextAndCutAsync()` naming
|
|
- 3 overloads of `PrintImageBitModeAsync()` and `LoadImageAsync()`
|
|
- Interface doesn't match all implementation overloads
|
|
- **Fix:** Rationalize method naming and signatures
|
|
|
|
---
|
|
|
|
## Suggested Refactoring Projects
|
|
|
|
### Project 1: Resource Management Overhaul
|
|
- Implement IAsyncDisposable properly across all classes
|
|
- Fix all HttpClient instantiation
|
|
- Add proper cleanup on shutdown
|
|
- **Scope:** ~10 files, ~200 lines changed
|
|
|
|
### Project 2: Async Pattern Modernization
|
|
- Add CancellationToken to all async methods
|
|
- Fix fire-and-forget patterns
|
|
- Replace synchronous waits
|
|
- **Scope:** ~8 files, ~150 lines changed
|
|
|
|
### Project 3: Thread Safety Fixes
|
|
- Fix race conditions in PrintServer and PrinterQueue
|
|
- Add proper locking for static initialization
|
|
- **Scope:** ~4 files, ~50 lines changed
|
|
|
|
### Project 4: Split EpsonPrinter Class
|
|
- Extract ConnectionManager
|
|
- Extract StatusProvider
|
|
- Extract ImagePrinter
|
|
- Keep EpsonPrinter as facade
|
|
- **Scope:** ~5 new files, ~800 lines reorganized
|
|
|
|
### Project 5: Configuration & DI Cleanup
|
|
- Add configuration validation
|
|
- Fix Logger factory singleton
|
|
- Separate configuration concerns
|
|
- Move secrets to environment variables
|
|
- **Scope:** ~5 files, ~100 lines changed
|
|
|
|
---
|
|
|
|
## Verification
|
|
|
|
After implementing any changes:
|
|
1. Build the solution: `dotnet build Inspectron.Epson.slnx`
|
|
2. Run existing tests: `dotnet test EpsonTest/EpsonTest.csproj`
|
|
3. Test printer connectivity manually with EpsonTest project
|
|
4. Verify systemd service starts/stops cleanly
|