search

apache / cordova-plugin-file-transfer

Apache Cordova File Transfer Plugin
https://cordova.apache.org/
Apache License 2.0
596 stars 887 forks source link

CB-12336 (android) Do not call win callback if aborted #171

Closed alsorokin closed 7 years ago

alsorokin commented 7 years ago

Platforms affected

Android

What does this PR do?

Fixes a rare condition on Android when win callback is called even after the abort(). Added a test to make sure the aborted flag is reset correctly at the next download attempt.

What testing has been done on this change?

~10 test runs on CI

Checklist

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 50 failed.

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 50 failed.

cordova-qa commented 7 years ago

Cordova CI Build has completed successfully.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 0 failed.

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 1 failed.

cordova-qa commented 7 years ago

Cordova CI Build has completed successfully.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 0 failed.

cordova-qa commented 7 years ago

Cordova CI Build has completed successfully.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

212 tests run, 12 skipped, 0 failed.

cordova-qa commented 7 years ago

Cordova CI Build has completed successfully.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

265 tests run, 13 skipped, 0 failed.

vladimir-kotikov commented 7 years ago

LGTM

filmaj commented 7 years ago

oo, nice catch!

However, I fail to see how the test added is exercising the abort() code path? I'm not actually sure what the test is doing other than ensuring a specific amount of data was loaded in. Can you elaborate on what the test is doing, @alsorokin ?

alsorokin commented 7 years ago

Hey @filmaj, nice to see you 😄

Nevermind this test, it was meant to ensure that my changes does not break downloading two files successively.

The bad news are that the fix is still not working: http://cordova-ci.cloudapp.net:8080/job/cordova-periodic-build-TEST/58/PLATFORM=android-4.4,PLUGIN=cordova-plugin-file-transfer/testReport/junit/cordova-plugin-file-transfer-tests.tests%20__.FileTransfer.methods/download/filetransfer_spec_9_should_not_leave_partial_file_due_to_abort/

After some investigation, I think that calling win callback after abort() is kind of expected behavior, at least if we've not calledfail callback yet and at the moment of abortion the file is ready or is being saved to disk or anything else.

Going to adapt the spec.9 to expect fail callback, but if we receive win, we just make sure there were no other fail callbacks called and get on with it.

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

0 tests run, 0 skipped, 0 failed.

alsorokin commented 7 years ago

Updated the branch. Going to test the new fix during the weekend.

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

0 tests run, 0 skipped, 0 failed.

cordova-qa commented 7 years ago

Cordova CI Build has one or more failures.

Commit - Link Dashboard - Link

Builder Name Console Output Test Report Device Logs
Windows 10 Store Link Link Link
iOS 9.3 Link Link Link
iOS 10.0 Link Link Link
Android 4.4 Link Link Link
Android 5.1 Link Link Link

0 tests run, 0 skipped, 0 failed.