abstract class BaseClientService supposed to be thread-safe, but is not

[geroj]

short description:

BaseClientService expose List Service.HttpClient.MessageHandler.ExceptionHandlers.Add(this);
This list can be accessed from several threads and cause application to crash.

long description:

I got several exceptions, when I used InsertMediaUpload request from namespace Google.Apis.Storage.v1 from multiple threads.

This class implements ResumableUpload, where entire upload is done.

Each exception was found in calling method UploadCoreAsync:

Object reference not set to an instance of an object.
   at Microsoft.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at Microsoft.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccess(Task task)
   at Microsoft.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Google.Apis.Upload.ResumableUpload`1.<UploadCoreAsync>d__e.MoveNext() in c:\code\google.com\google-api-dotnet-client\default\Tools\Google.Apis.Release\bin\Debug\test\default\Src\GoogleApis\Apis\[Media]\Upload\ResumableUpload.cs:l
ine 459

or

Collection was modified; enumeration operation may not execute.
   at Microsoft.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at Microsoft.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccess(Task task)
   at Microsoft.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Google.Apis.Upload.ResumableUpload`1.<UploadCoreAsync>d__e.MoveNext() in c:\code\google.com\google-api-dotnet-client\default\Tools\Google.Apis.Release\bin\Debug\test\default\Src\GoogleApis\Apis\[Media]\Upload\ResumableUpload.cs:line 459
Microsoft.Threading.Tasks

or

Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
   at System.Collections.Generic.List`1.RemoveAt(Int32 index)
   at System.Collections.Generic.List`1.Remove(T item)
   at Google.Apis.Upload.ResumableUpload`1.ServerErrorCallback.Dispose() in c:\code\google.com\google-api-dotnet-client\default\Tools\Google.Apis.Release\bin\Debug\test\default\Src\GoogleApis\Apis\[Media]\Upload\ResumableUpload.cs:line 273
   at Google.Apis.Upload.ResumableUpload`1.<UploadCoreAsync>d__e.MoveNext() in c:\code\google.com\google-api-dotnet-client\default\Tools\Google.Apis.Release\bin\Debug\test\default\Src\GoogleApis\Apis\[Media]\Upload\ResumableUpload.cs:line 463
mscorlib

Problem is in this part of code(method UploadCoreAsync) :
using (var callback = new ServerErrorCallback(this))
{
}

ServerErrorCallback is working with List ExceptionHandlers exposed by BaseClientService
Owner.Service.HttpClient.MessageHandler.UnsuccessfulResponseHandlers.Add(this);
Owner.Service.HttpClient.MessageHandler.ExceptionHandlers.Add(this);

Internal implementation (ConfigurableMessageHandler) is using List, so when I called code in UploadCoreAsync from several threads, it caused List to crash.

[geroj]

Hi,

Is there any progress on this issue?

[peleyal]

Hi.. sorry for the huge delay...
So, this one is hard, since by solving this one we are going to break the current interface, but my suggestion will be as following (and it's true to UnsuccessfulResponseHandlers, ExceptionHandlers and ExecuteInterceptors lists)

1. We need to find a PCL collection that is concurrent. I didn't find one yet, so the alternative will be when ever using one of those collection lock an object
2. Expose AddUnsuccessfulResponseHandler and RemoveUnsuccessfulResponseHandlers (same for other collections)
3. Remove the getters of those lists. No one outside should access them

It's a breaking change to the library. I'm a little bit concerned about it, but it will definitely fix the problem. We should solve it in the next weeks, if we break the API we will introduce it in 1.10.

I'll consider out options with jtattermusch@

Am I missing something? What are your thoughts about my suggestion geroj@?
Do you have a better proposal? I'll be happy to hear that...

Thanks!
Eyal

[peleyal]

I created #617 in order to solve this issue. I'm still not 100% sure it's the right way, but I would like to get your opinions.
geroj@, Jan and Linda feel free to comment on #617.

Thanks!
Eyal

[peleyal]

I just committed #617. So I'm closing this one

[peleyal]

Reopening this one, since we are still debating if we should break the existing API, with changing the return value of the list form IList to IEnumerable.

[peleyal]

I just submitted the change that isn't going to a introduce any breaking change in the coming release. We are safe to close this issue now.

[geroj]

Thank you for this fix! When is it going to be released? I understand that this change will be in version 1.10

[peleyal]

It was just released earlier this week.
Enjoy!

[geroj]

Great, nice present for Christmas!