Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deadlock when calling upload again from the success callback (FileSystemManager) #35

Open
tristandl opened this issue Mar 19, 2020 · 3 comments

Comments

@tristandl
Copy link

We're running into a strange issue using the file upload service. We need to upload 2 files, and mapped the success callback into a completion handling closure. We tried uploading the 2nd file from this handler and we get a deadlock and no file is uploaded. We're using a BLE transport.

We've reviewed the code in this library and can't really see a cause for it, and the solution we have is to dispatch the second call on a background queue. However, it seems like this should be supported because:

  • the first file is uploaded on the main thread
  • the upload takes places on a worker thread
  • the callback is called on the main thread
  • the second file is therefore also called on main thread (and from reviewing the code here, the locks are all released by the time the callback is called)

I can provide examples and references if this isn't sufficient to understand or recreate the situation, but I'm mostly concerned that we're doing something else wrong and this async dispatch is just sweeping things under the carpet.

Any thoughts/help appreciated, thank you. And great work on the library, it looks really solid!

@bgiori
Copy link
Contributor

bgiori commented Mar 20, 2020

Hi Tristan, thanks for submitting this issue!

After reading the code again myself, I would agree that a deadlock seems unlikely. Did you find the source of the deadlock? Or is deadlock just an educated guess based on symptoms?

So, if I understand the timeline correctly:

  1. Init a FileSystemManager and start first upload
  2. When the first upload succeeds, use the same FileSystemManager to call upload again with a new file from the first upload delegate's didUploadFinish callback.

It's quite interesting that it works if you dispatch the second file upload. I would also be interested in seeing what happens if you initialize a new FileSystemManager for the second upload.

I can dedicate a bit of time to this next week. In the meantime, if you could step through the second call to upload, even down into the BLE transporter, and identify where we are getting blocked, that would make my life a bit easier :)

@liangfenxiaodao
Copy link

I did more tests today, I tried to upload a file right after downloading a file and always got the error:

A file transfer is already in progress

Then I noticed that in the downloadCallback, it calls:

self.downloadDelegate?.download(of: self.fileName!, didFinish: self.fileData!)

and then

self.resetTransfer()

So, if I upload file immediately in the downloadCallback, the transferState will be .uploading and error will be thrown.

@bgiori
Copy link
Contributor

bgiori commented Apr 3, 2020

Thanks for looking into it more. The order of those two calls was recently changed in order to avoid a crash.

Commit: a4c95e7

PR: #33

At first glance, It looks like we should be copying the fileName and fileData into new variables, resetting the transfer (which will set state to none and clear the fileName and fileData class variables), and finally call the delegate with the copied file name and data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants