Instance management: Dispose of instance after 5s #765
Conversation
| disposed atomic.Bool | ||
| disposedTimes atomic.Int64 |
There was a problem hiding this comment.
So I had to change these since race detection complained about these due to concurrent reads/write in tests. In my head, given that we dispose within im.locker.Lock(cacheKey), there should only ever be one goroutine doing the creation of a new instance and calling Dispose hency why the instance itself shouldn't have to do any additional locking in their Dispose method.
…fter condition is satisfied
| @@ -17,6 +18,7 @@ var ( | |||
| Name: "active_instances", | |||
| Help: "The number of active plugin instances", | |||
| }) | |||
| disposeTTL = 5 * time.Second | |||
There was a problem hiding this comment.
Lets discuss what this should be
There was a problem hiding this comment.
if this TTL is to avoid multiple instances calling Get at the same time I think 5s is a good value
There was a problem hiding this comment.
Cool lets start with this and evaluate if it works or not
marefr
requested review from
wbrowne,
andresmgot and
xnyo
and removed request for
a team
There was a problem hiding this comment.
what happens if in the time window in which an instance is "marked" to be deleted and it's actually deleted another one is created? I guess that's what we are fixing but I am not sure I am seeing the fix. I am thinking on:
- An instance is marked to be deleted (in 5s)
- A new instance is created (after 1s)
- The instance gets deleted?
@andresmgot not sure I understand. With your example it should be fine given that a new instance is created and new requests coming in will use that instance. Then instance is deleted after 5s, but shouldn't be any usage left of this one. Does this make sense? |
There was a problem hiding this comment.
Interesting work, the changes make sense 👍 . The tricky part is nailing down the TTL, if it's too low we will encounter the same issue again.
In my head solution should work and it's simple and straightforward. I was also thinking maybe we can keep a counter of how many times an instance is retrieved, and decrement it when it's being "released". So adding a Release method to the instance manager alongside Get. Then the caller is responsible for defer-calling Release. This would allow us to call Dispose when the counter reaches 0 (so nobody is using a given instance). However this complicates things, changes the interface and breaks plugins that use manual instance management (but we are discouraging that), so probably not worth it, but wanted to hear your thoughts on this as well
Right, I thought that calling |
| @@ -17,6 +18,7 @@ var ( | |||
| Name: "active_instances", | |||
| Help: "The number of active plugin instances", | |||
| }) | |||
| disposeTTL = 5 * time.Second | |||
There was a problem hiding this comment.
if this TTL is to avoid multiple instances calling Get at the same time I think 5s is a good value
marefr
changed the title
What this PR does / why we need it:
Alternative to #762 and #763. This currently doesn't work because haven't put any effort to resolve the tests, but mainly wanted to show the changes in instance_manager.go
Which issue(s) this PR fixes:
Fixes #753
Special notes for your reviewer: