zipDirectory method returns before archiving is complete
sevenzees opened this issue · 11 comments
I discovered this issue when trying to use the onProgress feature of zipDirectory. I was trying to run zipDirectory in its own isolate and send onProgress updates back to the main isolate using ReceivePorts. I wrote my code to close the ReceivePort after zipDirectory completed, and found that the ReceivePort was closed before receiving any onProgress updates.
I debugged this and found that it is due to zipDirectory calling addDirectory (which is an async method) without awaiting it. The fix is to make zipDirectory an async method and to await addDirectory when calling it. (After making this change, the code works perfectly and onProgress updates are received as expected during the archive process.)
The one challenge with making this fix is that it is a breaking change. Anyone currently using zipDirectory will need to refactor their code to await zipDirectory, or their code will not work properly. Could consider renaming the zipDirectory method to something else (like zipDirectoryAsync) to force people to update their code.
Otherwise, if you don't want to fix this issue, the workaround is to just use create, addDirectory, and close directly instead of using zipDirectory.
Thanks for the info and the PR. I'll need to look over this more closely, see if I can figure something out about how to avoid breaking backwards compatibility.
I'm debating between solutions. Your proposed solution, which would have the breaking change at the top zipDirectory level, or having two sets of functions:
sync: zipDirectory / addDirectory / addFile
async: zipDirectoryAsync / addDirectoryAsync / addFileAsync
which would have the breaking change at the addDirectory / addFile level.
I think your solution is better, and just have one set of functions that is async. I don't want to do a major version change just for that, I wish I knew how many people would get affected by the change. I don't know why I spaced out making zipDirectory async when I added that.
I guess the least breaking change would be
async: zipDirectoryAsync / addDirectory / addFile
sync: zipDirectory / _addDirectory/ _addFile
where _addDirectory / _addFile are private just for sync zipDirectory use
You could also keep the zipDirectory function as-is for now to avoid having to increment the major version and add a @deprecated to it with a comment about the issue with using it. Then also create a zipDirectoryAsync as the recommended replacement.
That sounds like a good idea
There are other breaking changes I've been wanting to do. One of these days I'll have time to do them. Dart has changed so much since I first wrote this library, 10 years ago... (where has the time gone?!)
I guess the least breaking change would be async: zipDirectoryAsync / addDirectory / addFile sync: zipDirectory / _addDirectory/ _addFile where _addDirectory / _addFile are private just for sync zipDirectory use
This might be the best solution, though there's the extra work of implementing _addDirectory and _addFile
There are other breaking changes I've been wanting to do. One of these days I'll have time to do them. Dart has changed so much since I first wrote this library, 10 years ago... (where has the time gone?!)
I really appreciate the work you've done on this library!
I have that last solution implemented now, just some copy/paste. Maybe it will give me incentive to do the other breaking changes all together and do a major version update, to get that duplicated code cleaned up.
Though I'm sure I've made bigger breaking changes without major version updates before, I haven't been a very good public library steward.
I pushed the changes to git, so I'll close your PR. I decided to take out the Deprecated declaration to keep the dart analyzer from complaining, so there will be the two methods, zipDirectory and zipDirectoryAsync.
Sounds good! Thanks for the great work!