Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): enhance error handling and thread safety in log management #1013

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

skrashevich
Copy link
Contributor

This commit introduces improved error handling in the logHandler function within the API, ensuring that any errors encountered while writing to or resetting the memory log are properly logged and communicated back to the client with an appropriate HTTP status code. Additionally, the circularBuffer struct in the logging mechanism now incorporates a mutex to ensure thread-safe operations, particularly for the Reset method which has been updated to clear all chunks and reset read/write pointers under lock protection.

The changes include:

  • Logging errors and returning HTTP 500 (Internal Server Error) if writing the memory log to the response writer fails.
  • Similarly, logging errors and returning HTTP 500 if resetting the memory log fails.
  • Updating the HTTP method not allowed error response to set the "Allow" header correctly and use the more appropriate HTTP 405 (Method Not Allowed) status code.
  • Adding a mutex to the circularBuffer struct to protect concurrent access, particularly in the Reset method which now clears all buffer chunks safely.

This commit introduces improved error handling in the logHandler function within the API, ensuring that any errors encountered while writing to or resetting the memory log are properly logged and communicated back to the client with an appropriate HTTP status code. Additionally, the circularBuffer struct in the logging mechanism now incorporates a mutex to ensure thread-safe operations, particularly for the Reset method which has been updated to clear all chunks and reset read/write pointers under lock protection.

The changes include:
- Logging errors and returning HTTP 500 (Internal Server Error) if writing the memory log to the response writer fails.
- Similarly, logging errors and returning HTTP 500 if resetting the memory log fails.
- Updating the HTTP method not allowed error response to set the "Allow" header correctly and use the more appropriate HTTP 405 (Method Not Allowed) status code.
- Adding a mutex to the circularBuffer struct to protect concurrent access, particularly in the Reset method which now clears all buffer chunks safely.

These enhancements aim to improve the robustness and concurrency safety of the log management functionality, ensuring that the system can handle errors gracefully and maintain consistent state even in multi-threaded environments.
@AlexxIT
Copy link
Owner

AlexxIT commented Apr 22, 2024

  1. What's the point to clear all chunks? It's a blank operation.
  2. What's the point with return empty error from Reset? It's always empty, but you checking it for error in it.
  3. I don't think that this error app.MemoryLog.WriteTo can happen.
  4. I agree that Mutex can help. But you use it only in Reset function.

@skrashevich
Copy link
Contributor Author

  1. What's the point to clear all chunks? It's a blank operation.

for buffer state consistence in unexpected cases

  1. What's the point with return empty error from Reset? It's always empty, but you checking it for error in it.

it's just best practice

  1. I don't think that this error app.MemoryLog.WriteTo can happen.

"Anything that can go wrong will go wrong." © Murphy's law

  1. I agree that Mutex can help. But you use it only in Reset function.

By the nature of the ring buffer, potentially it can only be required in place where buffer's internal state is modified. IMHO

@AlexxIT
Copy link
Owner

AlexxIT commented Apr 22, 2024

  1. If we can't MemoryLog.WriteTo for some reason - we also can't write error after it.

It's all useless code. I don't see any sense in it, except http.StatusMethodNotAllowed.

@AlexxIT AlexxIT assigned AlexxIT and unassigned AlexxIT Apr 22, 2024
@AlexxIT AlexxIT added the doubt label Apr 22, 2024
Optimized the Bytes method in circularBuffer to precalculate the total length of the resulting byte slice, reducing allocations. Added tests and benchmarks for the Bytes method to ensure correctness and measure performance improvements.
@skrashevich
Copy link
Contributor Author

some upds

@AlexxIT
Copy link
Owner

AlexxIT commented May 12, 2024

I still don't see a problem with the log code. Adding mutexes will add blocking. But without them, there are no critical problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants