kothar / go-backblaze

A golang client for Backblaze's B2 storage
MIT License
94 stars 31 forks source link

add concurrent queue for reusing UploadAuth #23

Closed antihax closed 6 years ago

antihax commented 6 years ago

This was the simplest implementation i could think of.

It will keep a queue of 100 idle upload urls and create a new one only if one is not present on the queue. If one fails, it should not be added back to the queue.

I am not completely sure about the implementation of the return, but since UploadAuth is exported, it made sense to export return also.

Let me know what you think and modify as you see fit.

kothar commented 6 years ago

That looks excellent. I was considering use of sync.Pool, but various blogs seem to suggest a chan buffer as you have implemented unless you are mainly aiming to reduce allocation/garbage collection. I'm happy to merge as-is.

Thanks very much!

antihax commented 6 years ago

I tried sync.Pool, it frees the collection each GC cycle apparently.

If you feel it is good enough to merge I think we can go ahead. Just wondering if there needs to be an expulsion of the queue if the with invalidates over time. It will handle it, but could cause upto 100 errors in the process.

On Thu, Aug 30, 2018, 15:42 Mike Houston notifications@github.com wrote:

That looks excellent. I was considering use of sync.Pool, but various blogs seem to suggest a chan buffer as you have implemented unless you are mainly aiming to reduce allocation/garbage collection. I'm happy to merge as-is.

Thanks very much!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kothar/go-backblaze/pull/23#issuecomment-417460286, or mute the thread https://github.com/notifications/unsubscribe-auth/AGqJ25P5XZwqQHainKOTilsXVtoQTp5Mks5uWE4ygaJpZM4WSi6f .

kothar commented 6 years ago

Hmm, good point. I see a few of options: 1) Shorten the queue (1 entry?), assuming that we only need to keep hold of the authorisation long enough to pass to the next call 2) Expire the auth after a set period of time, reading from the queue until a valid token is found or the end reached 3) Automatically retry if the token was a reused one. The return method could mark the token. It still feels a bit racy if one goroutine was pooling invalid credentials and another trying to use them.

Do you have any preference in approach?

antihax commented 6 years ago

let me look at shortening, could drop to 10 or so or make it configurable? maxIdleUploads.

On Thu, Aug 30, 2018, 15:56 Mike Houston notifications@github.com wrote:

Hmm, good point. I see a few of options:

  1. Shorten the queue (1 entry?), assuming that we only need to keep hold of the authorisation long enough to pass to the next call
  2. Expire the auth after a set period of time, reading from the queue until a valid token is found or the end reached
  3. Automatically retry if the token was a reused one. The return method could mark the token. It still feels a bit racy if one goroutine was pooling invalid credentials and another trying to use them.

Do you have any preference in approach?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kothar/go-backblaze/pull/23#issuecomment-417464290, or mute the thread https://github.com/notifications/unsubscribe-auth/AGqJ2xllPutj0zrWFURG3nW3UreSg6u6ks5uWFFngaJpZM4WSi6f .

antihax commented 6 years ago

I think the configurable option may be better, I have something I tested last night and looks to work ok. That way if the user is just serializing requests, they can set it to one so the it checks in and out each time, or they can set 100+ if they have many concurrent workers.

A user could also manually drain the pool if needed if there will be significant time between usage.