fix: gcs manager cancels context #2662
Conversation
Codecov ReportBase: 43.73% // Head: 45.42% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2662 +/- ##
==========================================
+ Coverage 43.73% 45.42% +1.68%
==========================================
Files 191 287 +96
Lines 40483 47785 +7302
==========================================
+ Hits 17707 21708 +4001
- Misses 21671 24697 +3026
- Partials 1105 1380 +275
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -66,9 +66,6 @@ func (manager *GCSManager) ListFilesWithPrefix(ctx context.Context, startAfter, | |||
| return | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithTimeout(ctx, manager.getTimeout()) | |||
| defer cancel() | |||
|
|
|||
| // Create GCS Bucket handle | |||
| if manager.Config.Iterator == nil { | |||
There was a problem hiding this comment.
@BonapartePC why are we caching the iterator? Is this thread safe??
There was a problem hiding this comment.
The way this function is being used is: With a given manager, calling subsequent ListFilesWithPrefix() should return us continuous items one after the other. This is the reason we cache it in the manager itself.
There was a problem hiding this comment.
This is not safe, we need to revisit. Can we guarantee that filemanager will be used exclusively by one goroutine and then never reused?
There was a problem hiding this comment.
Currently, all of the code is written keeping that in mind - https://github.com/rudderlabs/rudder-server/blob/master/services/filemanager/s3manager.go#L207
There was a problem hiding this comment.
this is a bad api design, I am confident we can do much better than that
Description
GCS Manager creates an iterator to fetch objects. Bug: We were passing context to it and later cancelling at the end of the function call although the iterator will be used for every call of the function
Notion Ticket
https://www.notion.so/rudderstacks/Fix-GCS-Replay-2dd4e0f0e97942a5bcce282a5f61ea77
Security