CXuesong / WikiClientLibrary

/*🌻*/ Wiki Client Library is an asynchronous MediaWiki API client library targeting modern .NET platforms
https://github.com/CXuesong/WikiClientLibrary/wiki
Apache License 2.0
82 stars 16 forks source link

FilePage.UploadAsync does not have retry logic #10

Closed Silic0nS0ldier closed 7 years ago

Silic0nS0ldier commented 7 years ago

While performing some automatic page creation, I noticed that there were clusters of file upload failures. These failure clusters aligned with short periods of poor WiFi connectivity, and investigating the library code revealed that there was no retry logic, meaning that any dip in connectivity can result in several uploads failing.

CXuesong commented 7 years ago

Yes, this is by design because basically you cannot reliably re-read from an arbitrary Stream (though you surely can read again from some, e.g. FileStreams by seeking and reading again). On the other hand, it's not memory-efficient if I buffer the whole file into a, let's say, array first to guarantee the retry ability.

Thus I think I will enable retry for Stream whose CanSeek is true, namely, FileStream or MemoryStream or similar ones,

Silic0nS0ldier commented 7 years ago

Fair enough if there are technical constraints. That said, retry for Streams with CanSeek equal to true would be awesome.

CXuesong commented 7 years ago

I'm still working on this. As a note, I re-discovered why I had disabled retry in the first time. Though StreamContent does support re-read if the CanSeek of the provided Stream is true (I've check its source code), but HttpClient.SendAsync will automatically Dispose the passed in HttpContent instance, which means I still need to rebuild the HttpContent instance when retrying.

A part of the call stack that indicates the disposal behavior in HttpClient.SendAsync:

    System.Net.Http.dll!System.Net.Http.MultipartContent.Dispose(bool disposing)    未知
>   System.Net.Http.dll!System.Net.Http.HttpClient.SendAsync.AnonymousMethod__0(System.Threading.Tasks.Task<System.Net.Http.HttpResponseMessage> task)  未知
Silic0nS0ldier commented 7 years ago

I can see how that would create issues. No need to rush a fix or anything, I've got the bot task I needed to do done. Will probably be using this library again though (hence the feedback). Appreciate the transparency too by the way.

CXuesong commented 7 years ago

I think I've fixed this.

p.s.

Another observation: StreamContent will dispose the underlying Stream when StreamContent.Dispose is called, and there is actually no way to prevent it (like leaveOpen parameter in StreamReader's constrcutor), so I did some hacking stuff, because if the Stream got disposed, then there's nothing I can do for it XD