added onUploadProgress

[Mehmetyaz]

added onUploadProgress to MultipartRequest.new and Client.send. (adjusted file movements)

[Mehmetyaz]

@kevmoo - More than 80 changes appeared in the previous frok. With the new fork, your inspection will be easier.

[brianquinlan]

@natebosch , could you take a look at this?

[brianquinlan]

FYI, @natebosch is on holiday until late next week so don't expect feedback until then.

[natebosch]

@brianquinlan, @lrhn - how meaningful do we expect this to be? Is watching a transform step in a stream being read by dart:io HTTP handling an accurate way to understand upload progress?

[Mehmetyaz]

Not exactly. While I was writing the altogic_dart package, I duplicated the http package and I add this feature. Meanwhile, I split all yields larger than the specified chunk size according to this size. But adding this to the official package would have caused more confusion. Currently MultipartRequest splits into specific chunks and progress works correctly as the part is actually. But it needs to be divided into more accurate parts for big files.

E.g current results like this for big files : 0.1- 1-99.3-100

If large yields are divided, loading can be done with closer ratios.

[Mehmetyaz]

There is two methods for this.

1. Like browser client, read stream until done, split by defined chunk size. This may cause more memory consumption.

2. Split big parts by chunk size in the ByteStream. But this splitting will be done for browser also and it unnecessary. Because browser client works different.

If there is a solution you approve, I can code it. I also need to find out if the chuck size will be a static value or a parameter. Parameters can cause confusion. if it is static how much should it be?

[lrhn]

Or make total nullable and don't provide it when you don't have one.

If the loaded is defined as being bytes, at least when total is null, then the user can still be shown a meaningful progress, it'll just be "49 MB" instead of "37%".

I'd never read the entire stream and then chunk it afterwards. That's just useless overhead. If everything comes in one big chunk, that's just what you have to deal with.

[Mehmetyaz]

Making total nullable does not fix the problem. Instead, the ByteStream .fromBytes consturctor simply splits the List(bytes) into parts.

If MultipartFile is created with a stream, meaningful data is now obtained. But when created with fromBytes it pipes it as one piece. This prevents the rates from becoming meaningful.

Also at the moment onUploadProgress is not called with a rate anyway. There are loaded and total. Since the callback is triggered only where the total is calculated, the developer can calculate the rate.

[brianquinlan]

@brianquinlan, @lrhn - how meaningful do we expect this to be? Is watching a transform step in a stream being read by dart:io HTTP handling an accurate way to understand upload progress?

Probably not. When you write to a dart socket it doesn't mean that the data has actually left the local machine (or even been copied to OS memory).

[brianquinlan]

Of the 6 implementations that I know about, you could get good numbers for:

• browser
• cupertino_http

and bad numbers for:

• fetch_client (though this should improve with time as the JavaScript streaming API improves)
• cronet_http
• io
• java_http (not sure about this, based on Java HttpURLConnection)

[lrhn]

More words may make this easier to read (to a limit, as always).

/// When run in a browser, this callback relies on the
/// browser's XMLHttpRequest's xhr.upload.onLoad events.
///
/// When run natively, the number of bytes is
/// ....

I don't understand the description of the native version.
Probably because I don't know what stream we are talking about, and how you can possibly know the total length of a stream before it's done.

[Mehmetyaz]

MultipartRequest calculates the length value before the stream starts. If the length value is a non-calculated request (which I just added to the MultipartRequest), onUploadProgress will never be triggered.

Documentation will be extended

[brianquinlan]

@lrhn I actually prefer the original API (using a callback rather than a Stream) because it means that the Client implementation can easily check whether the caller is interesting in progress notification one at the beginning of their send implementation. In package:cupertino_http, I'd probably implement something like:

  @override
  Future<StreamedResponse> send(BaseRequest request) async {
    ...
    if (request.onUploadProgress != null) {
      // This `Timer` is an unnecessary expense if no one is listening for notifications.
      final timer = Timer.periodic(const Duration(milliseconds: 100), (timer) { 
      if (taskTracker.isDone) {
        timer.cancel();
      } else {
        progress = Progress(task.countOfBytesExpectedToSend,  task.countOfBytesSent);
        request.onUploadProcess(progress);
      });
    }

If the Stream/StreamController approach were used, it seems like the StreamController would have to be public and so the send implementation can do something like:

  @override
  Future<StreamedResponse> send(BaseRequest request) async {
    void setupTimer() {
    
    }

    if (request.controller.isListening) {
      setupTimer();
    } else {
      request.onListen = setupTimer;
   }

[lrhn]

Or make total nullable and don't provide it when you don't have one.

If the loaded is defined as being bytes, at least when total is null, then the user can still be shown a meaningful progress, it'll just be "49 MB" instead of "37%".

I'd never read the entire stream and then chunk it afterwards. That's just useless overhead. If everything comes in one big chunk, that's just what you have to deal with.

[AriasBros]

Hi @Mehmetyaz, thanks for this PR, it is very useful! We are using your fork in our app overriding the dependency of the http package until this get deployed in a new version of the package.

Said this, we noted that this signature could be wrong and has the parameters in the wrong order.
In the rest of the code you are calling the onUploadProgress callback using the total as the first parameter and the uploaded bytes as the second one.

Just that. Thanks a lot again.

[mehcode]

Is there anything needed here before this is merged? I'd really love to see this merged and support added to cupertino_http.

[kevmoo]

@brianquinlan @lrhn ?

[natebosch]

I'd still like to understand how meaningful these numbers are.
Moving the http implementation to the progress event helps - it feels like we're adding specific value in that case. If we feel like the stream watching is high enough fidelity for the IO case we can support both through the same interface. Are we confident in that fidelity @brianquinlan @lrhn ?
Is there any buffering in between this Stream that we can account for better with an API in the SDK? Will watching the Stream consumption be meaningful for the cronet and cupertino implementations?

If what we're offering is implemented as a callback that gets invoked as the Stream is consumed - does that need to be shipped as part of package:http?

[lrhn]

Could we provide a (broadcast) stream instead, instead of putting the callback directly into the class.

That's the traditional way to provide events, and the on name would be traditional for such a stream.

/// Upload progress notification
///
/// Sends events when upload progress information is
/// available.
///
/// ....
Stream<Progress> get onUploadProgress;

with a

abstract class Progress {
  int progress;
  int? get total;
}

(Which I'd make a record when we get records, and document as something users must not implement or extend.)

[Mehmetyaz]

If you agree, I can commit to implement this solution.

[Mehmetyaz]

Unfortunately I found that this is not a viable solution. What will I return in the getter you defined to BaseRequest? StreamController.stream.

A StreamController is required and if the developer does not want to listen to this progress, it creates an unnecessary load. In addition, since I need to know if the user has decided to listen for progress and this controller must be nullable.

If I could access the private values of BaseRequest from within the client (e.g IOClient), maybe I could come up with suitable solutions. Otherwise, two more properties are required, such as BaseRequest.onUploadProgress and BaseRequest.uploadProgressController?. I can guess that you don't want this visible to the developer.

[lrhn]

You can always create the controller when requested, and store it in a private variable:

  StreamController<Progress>? _controller;
  Stream<Progress> get onProgress => (_controller ??= StreamController()).stream;

  void _addProgress(int bytes, int? total) => _controller?.add(Progress(bytes, total));

This code has very little overhead when it's not used (a single null field and a single null check of that field whenever there is progress).
You can check whether _controller is null anywhere you currently check whether onProgressCallback is set.

If you need to access the internals inside IOClient, then you should just make the onProgress getter abstract and implement it in all the concrete subclasses. (I admit that the lack of protected access is annoying.)

Or add a

void addProgressToRequest(Progress progress, BaseRequest request) {
  request._addProgress(progress);
}

to base_progress.dart, use it from the other libraries, but don't export it as part of the public interface of the package.
(That's a traditional way to share privates between libraries in the same package.)

[brianquinlan]

AFAIK, we are waiting on a package:http major version bump to review changes like this.

Is that true @natebosch ?

[natebosch]

AFAIK, we are waiting on a package:http major version bump to review changes like this.

I haven't evaluated how difficult it will be to roll this internally. I do think it would need to wait for a major version bump.

I'm still a little unsure how meaningful these numbers are, or why it's important that we provide this API in this package instead of having the caller watch the stream progress.

[Mehmetyaz]

You are indeed right. In my implementation for Altogic, onUploadProgress was useful because I split the bytes into chunks even if bytes weren't streamed(e.g. multipartfile.fromBytes).

If it is not a problem for you, a solution is also possible, such as automatically increasing the number of stream parts in certain situations.

[brianquinlan]

I patched this change locally and did some testing.

For IOClient, the progress seemed useful for large requests (assuming that the stream is split into relatively small lists). On my Mac, ~500KiB of progress would be indicated before the first byte arrives on the server. So presumably that is approximately when the TCP write buffer is full [1].

For BrowserClient, I was not able go get any progress messages when running on Chrome - @Mehmetyaz how did you test this?

[1] I think that this can be verified by checking availableSendBuffer

[Mehmetyaz]

Moved to #1071