From f0ae32c9e58e980985b0ecab696c8976616f8ae1 Mon Sep 17 00:00:00 2001 From: EugeneTes Date: Thu, 5 Feb 2026 09:31:38 +0100 Subject: [PATCH] fallback support --- EpsonPrintService/Program.cs | 8 +- Inspectron.Epson/EpsonPrinter.cs | 6 +- .../PrintServer/IPrintJobSource.cs | 1 + .../JobSources/HttpPollingPrintJobSource.cs | 143 ++++++ .../JobSources/SignalRPrintJobSource.cs | 7 +- .../PrintServer/JobSources/SingleJobSource.cs | 5 + JUNIOR_ENGINEER_TASKS.md | 454 ------------------ plan.md | 262 ---------- 8 files changed, 165 insertions(+), 721 deletions(-) create mode 100644 Inspectron.Epson/PrintServer/JobSources/HttpPollingPrintJobSource.cs delete mode 100644 JUNIOR_ENGINEER_TASKS.md delete mode 100644 plan.md diff --git a/EpsonPrintService/Program.cs b/EpsonPrintService/Program.cs index a2c274b..38a2311 100644 --- a/EpsonPrintService/Program.cs +++ b/EpsonPrintService/Program.cs @@ -54,6 +54,8 @@ kernel.Bind().ToConstant(config); kernel.Bind().To(); kernel.Bind().To().InSingletonScope(); +//kernel.Bind().To().InSingletonScope(); + kernel.Bind().To(); kernel.Bind().ToMethod(ctx => { @@ -74,6 +76,7 @@ kernel.Bind().To() kernel.Bind().ToSelf().InSingletonScope(); kernel.Bind().ToSelf().InSingletonScope(); + var printLoop = kernel.Get(); var printServer = kernel.Get(); @@ -81,7 +84,7 @@ var printServer = kernel.Get(); var discoveryTask = kernel.Get(); var heartbeatTask = kernel.Get(); - +var jobSourceTask = kernel.Get(); //printServer.RegisterPrinter("10.0.20.12"); @@ -100,10 +103,13 @@ Console.CancelKeyPress += (sender, e) => _ = discoveryTask.StartAsync(cts.Token); _ = heartbeatTask.StartAsync(cts.Token); + // delay for a moment to allow discovery to find printers Console.WriteLine("Waiting for printer discovery..."); await Task.Delay(3000); +_ = jobSourceTask.StartAsync(); + await printLoop.StartAsync(); try diff --git a/Inspectron.Epson/EpsonPrinter.cs b/Inspectron.Epson/EpsonPrinter.cs index daa9e1a..8d0185c 100644 --- a/Inspectron.Epson/EpsonPrinter.cs +++ b/Inspectron.Epson/EpsonPrinter.cs @@ -12,7 +12,7 @@ namespace Inspectron.Epson; /// public class EpsonPrinter : IEpsonPrinter { - private readonly ILogger? _logger; + private readonly ILogger? _logger=null; private TcpClient? _tcpClient; private NetworkStream? _stream; private string? _printerIp; @@ -29,9 +29,9 @@ public class EpsonPrinter : IEpsonPrinter /// Initialize a new instance of EpsonPrinter /// /// Optional logger for diagnostic output - public EpsonPrinter(ILogger? logger = null) + public EpsonPrinter(/*ILogger? logger = null*/) { - _logger = logger; + //_logger = logger; RegisterCodepages(); } diff --git a/Inspectron.Epson/PrintServer/IPrintJobSource.cs b/Inspectron.Epson/PrintServer/IPrintJobSource.cs index 82ecf44..fc2437b 100644 --- a/Inspectron.Epson/PrintServer/IPrintJobSource.cs +++ b/Inspectron.Epson/PrintServer/IPrintJobSource.cs @@ -4,5 +4,6 @@ namespace Inspectron.Epson.PrintServer; public interface IPrintJobSource { + Task StartAsync(); Task GetNextJobAsync(CancellationToken cancellationToken); } \ No newline at end of file diff --git a/Inspectron.Epson/PrintServer/JobSources/HttpPollingPrintJobSource.cs b/Inspectron.Epson/PrintServer/JobSources/HttpPollingPrintJobSource.cs new file mode 100644 index 0000000..bc58b5a --- /dev/null +++ b/Inspectron.Epson/PrintServer/JobSources/HttpPollingPrintJobSource.cs @@ -0,0 +1,143 @@ +using System.Net.Http.Json; +using System.Text.Json; +using System.Text.Json.Serialization; +using System.Threading.Channels; +using Inspectron.Epson.PrintServer.ConfigurationSources; +using Inspectron.Epson.Queue; +using Microsoft.Extensions.Logging; + +namespace Inspectron.Epson.PrintServer.JobSources; + +public class HttpPollingPrintJobSource : IPrintJobSource, IDisposable +{ + private readonly EpsonPrintServiceConfiguration _groupConfiguration; + private readonly ILogger _logger; + private readonly HttpClient _httpClient; + private readonly Channel _printJobChannel = Channel.CreateUnbounded(); + private readonly CancellationTokenSource _pollingCts = new(); + private readonly TimeSpan _pollingInterval; + + public HttpPollingPrintJobSource( + EpsonPrintServiceConfiguration groupConfiguration, + ILogger logger +) + { + _groupConfiguration = groupConfiguration; + _logger = logger; + _pollingInterval = TimeSpan.FromSeconds(3); + + _httpClient = new HttpClient + { + BaseAddress = new Uri(groupConfiguration.ApiUrl?.TrimEnd('/') + "/") + }; + _httpClient.DefaultRequestHeaders.Add("X-Printer-Server-Key", groupConfiguration.ApiKey); + + } + + private async Task StartPollingAsync(CancellationToken cancellationToken) + { + _logger.LogInformation("Starting HTTP polling for print jobs at {BaseUrl}", _httpClient.BaseAddress); + + while (!cancellationToken.IsCancellationRequested) + { + try + { + await PollForJobsAsync(cancellationToken); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + break; + } + catch (Exception ex) + { + _logger.LogError(ex, "Error polling for print jobs. Retrying in {Interval}...", _pollingInterval); + } + + try + { + await Task.Delay(_pollingInterval, cancellationToken); + } + catch (OperationCanceledException) + { + break; + } + } + + _logger.LogInformation("HTTP polling stopped."); + } + + private async Task PollForJobsAsync(CancellationToken cancellationToken) + { + var requestUrl = $"/api/printerServer/jobs"; + + var response = await _httpClient.GetAsync(requestUrl, cancellationToken); + + if (!response.IsSuccessStatusCode) + { + _logger.LogWarning("Failed to fetch jobs. Status: {StatusCode}", response.StatusCode); + return; + } + + var jobs = await response.Content.ReadFromJsonAsync>(cancellationToken: cancellationToken); + + if (jobs == null || jobs.Count == 0) + { + return; + } + + _logger.LogInformation("Received {Count} print job(s) from HTTP endpoint.", jobs.Count); + + foreach (var job in jobs) + { + _logger.LogInformation("Queuing print job: {PrintJob}", JsonSerializer.Serialize(job)); + _printJobChannel.Writer.TryWrite(new PrintJob + { + JobId = job.JobId, + IP = job.PrinterIp, + Document = new SignalRPrintJobSource.PrintJobFromSignalR + { + JobId = job.JobId, + PrinterIp = job.PrinterIp, + LogoUrl = job.LogoUrl, + ReceiptType = job.ReceiptType, + Content = job.Content + } + }); + } + } + + public Task StartAsync() + { + return StartPollingAsync(_pollingCts.Token); + } + + public async Task GetNextJobAsync(CancellationToken cancellationToken) + { + return await _printJobChannel.Reader.ReadAsync(cancellationToken); + } + + public void Dispose() + { + _pollingCts.Cancel(); + _pollingCts.Dispose(); + _httpClient.Dispose(); + } + + public class PrintJobDto + { + [JsonPropertyName("jobId")] + public string JobId { get; set; } = ""; + + [JsonPropertyName("printerIp")] + public string PrinterIp { get; set; } = ""; + + [JsonPropertyName("logoUrl")] + public string? LogoUrl { get; set; } + + [JsonPropertyName("receiptType")] + public int ReceiptType { get; set; } + + [JsonPropertyName("content")] + public string Content { get; set; } = ""; + } +} diff --git a/Inspectron.Epson/PrintServer/JobSources/SignalRPrintJobSource.cs b/Inspectron.Epson/PrintServer/JobSources/SignalRPrintJobSource.cs index 98da9a7..008b5ab 100644 --- a/Inspectron.Epson/PrintServer/JobSources/SignalRPrintJobSource.cs +++ b/Inspectron.Epson/PrintServer/JobSources/SignalRPrintJobSource.cs @@ -34,7 +34,7 @@ public class SignalRPrintJobSource: IPrintJobSource _connection.Reconnected += OnReconnected; _connection.Closed += OnClosed; - _ = InitializeConnectionAsync(); + } private async Task InitializeConnectionAsync() @@ -106,6 +106,11 @@ public class SignalRPrintJobSource: IPrintJobSource }); } + public Task StartAsync() + { + return InitializeConnectionAsync(); + } + public async Task GetNextJobAsync(CancellationToken cancellationToken) { return await _printJobChannel.Reader.ReadAsync(cancellationToken); diff --git a/Inspectron.Epson/PrintServer/JobSources/SingleJobSource.cs b/Inspectron.Epson/PrintServer/JobSources/SingleJobSource.cs index fbba865..b6a2fc0 100644 --- a/Inspectron.Epson/PrintServer/JobSources/SingleJobSource.cs +++ b/Inspectron.Epson/PrintServer/JobSources/SingleJobSource.cs @@ -11,6 +11,11 @@ public class SingleJobSource: IPrintJobSource } private Channel _channel = Channel.CreateUnbounded(); + public Task StartAsync() + { + return Task.CompletedTask; + } + public async Task GetNextJobAsync(CancellationToken cancellationToken) { return await _channel.Reader.ReadAsync(cancellationToken); diff --git a/JUNIOR_ENGINEER_TASKS.md b/JUNIOR_ENGINEER_TASKS.md deleted file mode 100644 index 7b9bbd1..0000000 --- a/JUNIOR_ENGINEER_TASKS.md +++ /dev/null @@ -1,454 +0,0 @@ -# Junior Engineer Task List - Code Improvements - -This document contains actionable tasks derived from the code analysis in `plan.md`. Complete tasks in priority order. After each fix, run: -```bash -dotnet build Inspectron.Epson.slnx -dotnet test EpsonTest/EpsonTest.csproj -``` - ---- - -## CRITICAL PRIORITY (Fix First) - -### Task 1: Move API Key to Environment Variable -**File:** `EpsonPrintService/config.json:4` - -**Problem:** API key stored in plaintext - security risk. - -**Steps:** -1. Remove `ApiKey` from `config.json` -2. In `EpsonPrintService/Program.cs`, read from environment: -```csharp -var apiKey = Environment.GetEnvironmentVariable("EPSON_API_KEY") - ?? throw new InvalidOperationException("EPSON_API_KEY environment variable not set"); -``` -3. Update `EpsonPrintServiceConfiguration` class to accept apiKey via constructor or property -4. Add to systemd service file `epson.service`: -```ini -[Service] -Environment="EPSON_API_KEY=your-key-here" -``` -5. Document the required environment variable in README - ---- - -### Task 2: Fix HttpClient Anti-Pattern -**Files:** -- `EpsonPrintService/HeartbeatBackgroundTask.cs:24` -- `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:68` - -**Problem:** Creating new HttpClient per request causes socket exhaustion. - -**Steps:** -1. In `HeartbeatBackgroundTask.cs`: - - Change constructor to accept `HttpClient` as parameter - - Remove `_httpClient = new HttpClient();` line - - Keep using `_httpClient` field - -2. In `Program.cs` where HeartbeatBackgroundTask is created: -```csharp -// Create ONE HttpClient instance for the app lifetime -var httpClient = new HttpClient(); -var heartbeatTask = new HeartbeatBackgroundTask(httpClient, ...); -``` - -3. In `EpsonPrintService.cs:68`: - - Add `HttpClient _httpClient` field to class - - Inject via constructor - - Replace `using var httpClient = new HttpClient();` with `_httpClient` - ---- - -### Task 3: Add Graceful Shutdown -**File:** `EpsonPrintService/Program.cs:51-82` - -**Problem:** PrintLoop and PrintServer never call StopAsync() - jobs lost on Ctrl+C. - -**Steps:** -1. Store references to started services: -```csharp -var printLoop = kernel.Get(); -var printServer = kernel.Get(); -``` - -2. In the Console.CancelKeyPress handler (or before Environment.Exit): -```csharp -Console.CancelKeyPress += async (sender, e) => -{ - e.Cancel = true; // Prevent immediate exit - Console.WriteLine("Shutting down gracefully..."); - - await printLoop.StopAsync(); - await printServer.StopAsync(); - - cts.Cancel(); -}; -``` - -3. Ensure `PrintLoop` and `PrintServer` have `StopAsync()` methods that properly clean up - ---- - -### Task 4: Add Null Check for PrinterId -**File:** `Inspectron.Epson/PrintServer/PrintServices/EpsonPrintService.cs:35-44` - -**Problem:** `GetPrinterIdAsync()` can return null but `.Value` accessed without check. - -**Steps:** -1. Find this code block: -```csharp -var printerId = await epsonPrinter.GetPrinterIdAsync(); -var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...); -``` - -2. Add null check: -```csharp -var printerId = await epsonPrinter.GetPrinterIdAsync(); -if (printerId == null || !printerId.HasValue) -{ - _logger.LogError("Failed to get printer ID from {PrinterIp}", printerIp); - return false; -} -var printerAdapter = _wrapperPrinterFactory.CreatePrinterFromId(printerId.Value, ...); -``` - ---- - -### Task 5: Fix Race Condition in PrintServer -**File:** `Inspectron.Epson/Queue/PrintServer.cs:19-23` - -**Problem:** TOCTOU bug - two threads could create duplicate queues. - -**Steps:** -1. Change dictionary type from `Dictionary` to: -```csharp -private readonly ConcurrentDictionary _printerQueues = new(); -``` - -2. Replace this pattern: -```csharp -if (_printerQueues.ContainsKey(printerIp)) - return; -var queue = new PrinterQueue(...); -_printerQueues[printerIp] = queue; -``` - -With atomic operation: -```csharp -var queue = _printerQueues.GetOrAdd(printerIp, ip => -{ - var newQueue = new PrinterQueue(ip, _printService, _logger); - newQueue.StartAsync(); // Start the queue - Console.WriteLine($"Printer {ip} registered"); - return newQueue; -}); -``` - -3. Add `using System.Collections.Concurrent;` at top of file - ---- - -## HIGH PRIORITY - -### Task 6: Add CancellationToken Support -**File:** `Inspectron.Epson/EpsonPrinter.cs` - Lines 52, 96, 129, 205, 335, 408, etc. - -**Problem:** Async methods can't be cancelled by callers. - -**Steps:** -1. Find all public async methods in EpsonPrinter.cs -2. Add optional parameter to each: -```csharp -// Before: -public async Task ConnectAsync(string host, int port = 9100) - -// After: -public async Task ConnectAsync(string host, int port = 9100, CancellationToken cancellationToken = default) -``` - -3. Pass cancellationToken to internal async calls: -```csharp -await _stream.WriteAsync(data, cancellationToken); -await _stream.ReadAsync(buffer, cancellationToken); -``` - -4. Update interface `IEpsonPrinter` if one exists - ---- - -### Task 7: Add Exception Handling to Fire-and-Forget Tasks -**Files:** -- `Inspectron.Epson/PrintServer/PrintLoop.cs:29` -- `EpsonPrintService/Program.cs:71-72` - -**Problem:** Unobserved exceptions crash the process. - -**Steps:** -1. In `PrintLoop.cs`, replace: -```csharp -_ = Task.Run(Loop); -``` -With: -```csharp -_ = Task.Run(async () => -{ - try - { - await Loop(); - } - catch (Exception ex) - { - _logger.LogError(ex, "PrintLoop crashed unexpectedly"); - } -}); -``` - -2. In `Program.cs`, replace: -```csharp -_ = discoveryTask.StartAsync(cts.Token); -_ = heartbeatTask.StartAsync(cts.Token); -``` -With: -```csharp -var discoveryRunning = Task.Run(async () => -{ - try { await discoveryTask.StartAsync(cts.Token); } - catch (Exception ex) { Console.WriteLine($"Discovery error: {ex.Message}"); } -}); - -var heartbeatRunning = Task.Run(async () => -{ - try { await heartbeatTask.StartAsync(cts.Token); } - catch (Exception ex) { Console.WriteLine($"Heartbeat error: {ex.Message}"); } -}); -``` - ---- - -### Task 8: Implement IDisposable for Resource Cleanup -**Files:** -- `EpsonPrintService/HeartbeatBackgroundTask.cs` -- `Inspectron.Epson/Queue/PrinterQueue.cs:13-14` -- `Inspectron.Epson/EpsonPrinter.cs:757-761` - -**Steps:** - -1. **HeartbeatBackgroundTask.cs** - Add IDisposable: -```csharp -public class HeartbeatBackgroundTask : IDisposable -{ - private bool _disposed; - - public void Dispose() - { - if (_disposed) return; - _httpClient?.Dispose(); - _disposed = true; - } -} -``` - -2. **PrinterQueue.cs** - Add IDisposable: -```csharp -public class PrinterQueue : IDisposable -{ - private bool _disposed; - - public void Dispose() - { - if (_disposed) return; - _semaphore?.Dispose(); - _cts?.Dispose(); - _disposed = true; - } -} -``` - -3. **EpsonPrinter.cs** - Fix DisposeAsync: -```csharp -public async ValueTask DisposeAsync() -{ - Disconnect(); - GC.SuppressFinalize(this); - await Task.CompletedTask; -} -``` - ---- - -### Task 9: Replace .Wait() with await -**File:** `Inspectron.Epson/Queue/PrintServer.cs:32` - -**Problem:** `.Wait()` can cause deadlocks. - -**Steps:** -1. Find method containing: -```csharp -queue.StopAsync().Wait(); -``` - -2. Make the method async: -```csharp -// Before: -public void StopQueue(string printerIp) - -// After: -public async Task StopQueueAsync(string printerIp) -``` - -3. Replace `.Wait()` with `await`: -```csharp -await queue.StopAsync(); -``` - -4. Update all callers to use `await StopQueueAsync()` - ---- - -### Task 10: Add Configuration Validation at Startup -**File:** `EpsonPrintService/Program.cs:20-21` - -**Problem:** No validation after deserializing config. - -**Steps:** -1. After config deserialization, add validation: -```csharp -var config = JsonSerializer.Deserialize(configJson); - -if (config == null) - throw new InvalidOperationException("Failed to parse config.json"); - -if (string.IsNullOrEmpty(config.GroupId)) - throw new InvalidOperationException("GroupId is required in config.json"); - -if (string.IsNullOrEmpty(config.RestaurantId)) - throw new InvalidOperationException("RestaurantId is required in config.json"); - -if (config.PrinterConfigurations == null || config.PrinterConfigurations.Count == 0) - Console.WriteLine("Warning: No printer configurations found"); -``` - ---- - -## MEDIUM PRIORITY - -### Task 11: Replace Console.WriteLine with ILogger -**File:** `Inspectron.Epson/Queue/PrintServer.cs:25, 34, 64` - -Replace all `Console.WriteLine()` calls with proper logging: -```csharp -// Before: -Console.WriteLine($"Printer {printerIp} registered"); - -// After: -_logger.LogInformation("Printer {PrinterIp} registered", printerIp); -``` - ---- - -### Task 12: Fix Logger Factory Singleton -**File:** `EpsonPrintService/Program.cs:33-40` - -Create factory once: -```csharp -// Create once at startup -using var loggerFactory = LoggerFactory.Create(builder => -{ - builder.AddConsole(); - builder.SetMinimumLevel(LogLevel.Debug); -}); - -// Bind as singleton for all injections -kernel.Bind().ToConstant(loggerFactory); -kernel.Bind(typeof(ILogger<>)).ToMethod(ctx => - loggerFactory.CreateLogger(ctx.Request.Service.GetGenericArguments()[0])); -``` - ---- - -### Task 13: Extract Magic Numbers to Constants -**File:** `Inspectron.Epson/Queue/PrinterQueue.cs:87-88` - -```csharp -// Add at top of class: -private const int MaxRetryCount = 3; - -// Replace: -if (job.RetryCount >= 3) -// With: -if (job.RetryCount >= MaxRetryCount) -``` - ---- - -### Task 14: Fix Static Thread Safety -**File:** `Inspectron.Epson/EpsonPrinter.cs:38-43` - -```csharp -private static readonly object _initLock = new(); -private static bool _initialized; - -private static void RegisterCodepages() -{ - if (_initialized) return; - - lock (_initLock) - { - if (_initialized) return; // Double-check pattern - Encoding.RegisterProvider(CodePagesEncodingProvider.Instance); - _initialized = true; - } -} -``` - ---- - -## LOW PRIORITY - -### Task 15: Remove Commented-Out Code -- `EpsonPrintService/Program.cs:27-28` - Delete old factory bindings -- `Inspectron.Epson/PrintServer/Printers/TM-T30III.cs:25` - Delete dead code - -### Task 16: Resolve TODO Comments -**File:** `Inspectron.Epson/Queue/PrintJob.cs:5` -- Either change `IP` property to proper type or remove the TODO comment - -### Task 17: Remove Unused Serilog Packages -**File:** `EpsonPrintService/EpsonPrintService.csproj:14-16` -```bash -dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog -dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog.Extensions.Logging -dotnet remove EpsonPrintService/EpsonPrintService.csproj package Serilog.Sinks.Console -``` - -### Task 18: Fix Inconsistent Logging Style -Change string interpolation to structured logging: -```csharp -// Before: -_logger.LogWarning($"Printer {printerIp} not found"); - -// After: -_logger.LogWarning("Printer {PrinterIp} not found", printerIp); -``` - ---- - -## Verification Checklist - -After completing all tasks: - -- [ ] `dotnet build Inspectron.Epson.slnx` - No errors -- [ ] `dotnet test EpsonTest/EpsonTest.csproj` - All tests pass -- [ ] Test printer connectivity manually -- [ ] Verify systemd service starts/stops cleanly -- [ ] Verify graceful shutdown with Ctrl+C -- [ ] Check no Console.WriteLine in library code (only logging) - ---- - -## Questions? - -If you're unsure about any task: -1. Read the original issue in `plan.md` for more context -2. Look at existing patterns in the codebase -3. Ask a senior engineer before making architectural changes diff --git a/plan.md b/plan.md deleted file mode 100644 index 51f5d8a..0000000 --- a/plan.md +++ /dev/null @@ -1,262 +0,0 @@ -# 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(...); -// 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