Skip to content

fix: ensure setting/getting additional attributes is concurrency safe#66

Open
techfg wants to merge 1 commit intogo-chi:masterfrom
techfg:fix/concurrent-attribute-access
Open

fix: ensure setting/getting additional attributes is concurrency safe#66
techfg wants to merge 1 commit intogo-chi:masterfrom
techfg:fix/concurrent-attribute-access

Conversation

@techfg
Copy link

@techfg techfg commented Jun 27, 2025

Ensures that reading/writing to log attributes is concurrency safe.

Resolves #65

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

Thanks for raising #65!

While this looks good and solves the report. Can we think about a solution without using any extra locks?

@techfg
Copy link
Author

techfg commented Aug 8, 2025

Thanks for taking a look at this @VojtechVitek!

Here are some ideas but it really comes down to what the use cases are which you'd likely know better than I, however I can draw some assumptions since the patterns would be pretty standard:

  1. Leave as-is, avoid the locks added in this PR and instead document that the package is not concurrency safe - The downside to this is that: 1) There are use cases where concurrent access is leveraged; 2) Most people won't read the docs 😦; 3) The pattern to use the package and apply a concurrency safe wrapper would be less than ideal (although possible).
  2. Change to Mutex from RWMutex - Since most use cases are likely non-concurrent, using Mutex will have a slightly better performance profile than RWMutex. I applied RWMutex based on some assumptions that middleware might write a value and then go routines downstream would be reading that value. If locking is added, which synchronization type to use would come down to aligning with the most common use case
  3. Create a 2nd type (e.g., RequestLoggerSafe - need to think of a better name lol) that is strictly intended for use cases that require concurrent access

Those are just some ideas off the cuff but open to any that you have 😄 My vote would be to add locking, either Mutex or RWMutex depending on which use case is the most common as it keeps the API the same, doesn't introduce any requirements for users to handle concurrency on their own, and ensures that data integrity is maintained across all use cases.

Question - Other than what should be a very negligible performance impact (e.g., 10-100ns w/ a slight CPU & memory hit) which I do understand would be ideal to avoid entirely, can you expand on your reasons for not wanting to introduce locking?

@techfg
Copy link
Author

techfg commented Aug 8, 2025

  1. Leave as-is, avoid the locks added in this PR and instead document that the package is not concurrency safe - The downside to this is that: 1) There are use cases where concurrent access is leveraged; 2) Most people won't read the docs 😦; 3) The pattern to use the package and apply a concurrency safe wrapper would be less than ideal (although possible).
  2. Change to Mutex from RWMutex - Since most use cases are likely non-concurrent, using Mutex will have a slightly better performance profile than RWMutex. I applied RWMutex based on some assumptions that middleware might write a value and then go routines downstream would be reading that value. If locking is added, which synchronization type to use would come down to aligning with the most common use case
  3. Create a 2nd type (e.g., RequestLoggerSafe - need to think of a better name lol) that is strictly intended for use cases that require concurrent access

I thought of a 4th option that might be a good compromise....

  1. Make synchronization optional via Options by adding a property (e.g., Safe bool defaulting to false) and using a *sync.RWMutex (or *sync.Mutex) to logData which was added in this PR. During construction, if Safe is true, create a sync.RWMutex, else do not. Then, in get/set functions, check if nil and if so, don't synchronize, else synchronize. The docs should still be updated in this case to clearly indicate default behavior is not concurrency safe and if required, to set Safe=true:
    1. Pros:
      1. Leaves API signature as-is
      2. Leaves default functionality as-is
      3. Provides an explicit way to control concurrency safety when needed
      4. Minimal increase to default behavior memory & cpu utilization (8 bytes total memory additional & negligible CPU additional)
    2. Cons:
      1. Adds a ptr to memory footprint (via logData) for the default behavior but this is only 8 bytes in default behavior scenario
      2. Adds an extremely minimal conditional check on every get/set to see if ptr != nil but there is no additional memory used and the CPU utilization is extremely neglibile

This really is a variant of option 3 but doesn't increase code maintenance cost as much and leverages all the existing types that already exist. I'm in favor of this option vs. the other options I listed as I think it's the best compromise to what I suspect are your concerns around locking, although still curious to confirm what your reservations are along that line, if any, beyond performance :)

Look forward to your thoughts, thank you!

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.

SetAttrs does not handle concurrent writes

2 participants