search

brendan-duncan / archive

Dart library to encode and decode various archive and compression formats, such as Zip, Tar, GZip, ZLib, and BZip2.
MIT License
395 stars 130 forks source link

zipDirectory method returns before archiving is complete #292

Closed sevenzees closed 9 months ago

sevenzees commented 9 months ago

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.

brendan-duncan commented 9 months ago

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.

brendan-duncan commented 9 months ago

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.

brendan-duncan commented 9 months ago

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

sevenzees commented 9 months ago

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.

brendan-duncan commented 9 months ago

That sounds like a good idea

brendan-duncan commented 9 months ago

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?!)

sevenzees commented 9 months ago

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

sevenzees commented 9 months ago

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!

brendan-duncan commented 9 months ago

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.

brendan-duncan commented 9 months ago

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.

sevenzees commented 9 months ago

Sounds good! Thanks for the great work!