bertrandmartel / speed-test-lib

:cloud: JSpeedTest : speed test client library for Java/Android
MIT License
381 stars 119 forks source link

Possible concurrency issue when starting upload #11

Closed tiwiz closed 7 years ago

tiwiz commented 8 years ago

Hello,

I noticed something a bit strange: when starting the upload, often I get very wrong values (such as 200 Mbps in lines where the average is 1). It usually takes 3 to 4 "onUpdateProgress" call to stabilise it.

Here's an extract of the measurement I get: D/SpeedTestPresenter: Current speed of 0,00 Mbps. D/SpeedTestPresenter: Current speed of 50,00 Mbps. D/SpeedTestPresenter: Current speed of 62,50 Mbps. D/SpeedTestPresenter: Current speed of 76,92 Mbps. D/SpeedTestPresenter: Current speed of 2,53 Mbps. D/SpeedTestPresenter: Current speed of 1,39 Mbps. D/SpeedTestPresenter: Current speed of 1,07 Mbps. D/SpeedTestPresenter: Current speed of 0,92 Mbps. D/SpeedTestPresenter: Current speed of 0,83 Mbps. D/SpeedTestPresenter: Current speed of 0,77 Mbps. D/SpeedTestPresenter: Current speed of 0,72 Mbps. D/SpeedTestPresenter: Current speed of 0,67 Mbps. D/SpeedTestPresenter: Current speed of 0,65 Mbps. D/SpeedTestPresenter: Current speed of 0,63 Mbps. D/SpeedTestPresenter: Upload: MAX 76,92 Mbps - AVG 14,26 Mbps

bertrandmartel commented 8 years ago

Hi, thank you for reporting this. From the title concurrency issue, do you have this issue when issuing multiple upload request on the same object ? Is 14,26 Mbps in your example the result from onUploadPacketsReceived ?

tiwiz commented 8 years ago

No, the average is a calculated on the list of values I get. At the moment I only do one upload... From the onUploadPacketsReceived I can get the average as well? That would be very cool!

bertrandmartel commented 8 years ago

Yes onUploadPacketsReceived gives you the total average on the speed test duration. In onProgress callback that only gives you current average at that time of the speed test

@Override
public void onUploadPacketsReceived(int packetSize, float transferRateBitPerSeconds, 
                                    float transferRateOctetPerSeconds) {
    System.out.println("this is the average in bps     : " + 
               transferRateBitPerSeconds + " bps");
    System.out.println("this is the average in octet   : " +
               transferRateOctetPerSeconds + "Bps");
}
bertrandmartel commented 8 years ago

I cant reproduce your weird large transfer rate values at the beginning of upload test (I tried nested upload infinite loop), can you share how do you reproduce it ?

tiwiz commented 8 years ago

Sure, I use these routines:


    override fun onDownloadCompleted() {
        Timber.d("Download: MAX ${downloadCallback.max().format(2)} Mbps - AVG ${downloadCallback.avg().format(2)} Mbps")
        view.showUploadIcon()
        resetUi()
        startUpload(1000000)
    }

    private fun resetUi() {
        view.updateProgressTo(0)
        view.updateGaugeTo(0)
        view.updateSpeedWith("")
    }

    private fun startUpload(octet: Int = 10000000) {
        Observable.fromCallable { socket.startUpload(host, port, uploadFile, octet) }
                .delay(3, TimeUnit.SECONDS)
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe()
    }
bertrandmartel commented 8 years ago

Do you still have the issue if you remove UI operation or other blocking from all the SpeedTestListener callback ? In you onProgress do you perform UI or blocking stuff ? As the content of onProgress is running in writing thread for upload and reading thread for download, all blocking operation should be run in a different thread

tiwiz commented 8 years ago

Actually, the callbacks are the only thing happening on the main thread, all the rest is moved from the main thread.

I tried anyway to remove everything and just calling the upload and it does the same issue anyway. Attaching a debugger and setting a breakpoint in the onProgress doesn't trigger the issue, though.

bertrandmartel commented 8 years ago

Just to be sure it is not network specific

git clone git://github.com/akinaru/speed-test-lib.git
cd speed-test-lib
./gradlew uploadFile

Do you confirm that you have the same behavior with that test ? Thank you very much for your time

tiwiz commented 8 years ago

Yup,

it looks like I'm on a 500Mbps line (at work I'm on a 100Mbps one)

tiwiz commented 7 years ago

Short update: I managed to fix the issue on Android and I opened a PR :-)

bertrandmartel commented 7 years ago

Hello, thank you for the PR, I've implemented a method to set the setup time for download/upload

The setup time is the time from which the speed test rate is calculated as you've defined in your PR

This would take into account your issue and TCP congestion problem but would require to manually set this setup time.