Get uploaded size while upload is in progress #3661
Conversation
fredericBregier
force-pushed
the
master-upload-progress
branch
2 times, most recently
from
6c098ee
to
3ce04d5
Compare
|
Additionnal note: it seems we do not send any Content-Length in the encoder. This lead to an unkown final size for the transmitted file (or attribute). Continue to investigate and if someone has a clue... ;-) |
|
According to RFC2046 chapter 5.1: So adding Content-Length to Multipart is valid but not mandatory. I create another pull request for 3.10 since it is easier for me to test (no need of Groovy to build netty through Eclipse so direct testing is possible). See #3663 from now on, until accepted. Then I will redo the work for master version (and supposesly the same for 4.0 and 4.1 branches). |
|
Note that I am not (yet) able to locally test the Junit since now Netty contains a partial construction of source through Groovy scripts, therefore preventing Eclipse environment to build and run Junit tests. If someone has an idea how to managed this? |
|
Note for others: I answer myself since I managed finally... |
|
@normanmaurer @Scottmitch @trustin |
| * | ||
| * If no Content-Length is provided in the request, this is equivalent to the current size (length()) since the | ||
| * decoding does not known the final size yet. Therefore the defined length is | ||
| * always 0 the current length (whatever during decoding or in final state). |
There was a problem hiding this comment.
Therefore the defined length is always 0 the current length (whatever during decoding or in final state).
Can you clarify this. I'm not sure what is always 0 the current length means.
There was a problem hiding this comment.
Indeed it is not clear (and maybe even false). Better should be:
If no Content-Length is provided in the request, the defined length is always 0 (whatever during decoding or in final state).
|
@fredericBregier - I did a quick first pass. I would need to dig in to absorb this area a bit more...I'm not too familiar with this area of code. Perhaps I'll have more time later this week or this weekend... |
|
@Scottmitch Thanks for this first pass! I've corrected (in better English I hope). |
|
@Scottmitch @normanmaurer ping (and not to forget the 3.10 version #3663 ) |
Proposal to fix issue netty#3636 Motivations: Currently, while adding the next buffers to the decoder (`decoder.offer()`), there is no way to access to the current HTTP object being decoded since it can only be available currently once fully decoded by `decoder.hasNext()`. Some could want to know the progression on the overall transfer but also per HTTP object. While overall progression could be done using (if available) the global Content-Length of the request and taking into account each HttpContent size, the per HttpData object progression is unknown. Modifications: 1) For HTTP object, `AbstractHttpData` has 2 protected properties named `definedSize` and `size`, respectively the supposely final size and the current (decoded until now) size. This provides a new method `definedSize()` to get the current value for `definedSize`. The `size` attribute is reachable by the `length()` method. Note however there are 2 different ways that currently managed the `definedSize`: a) `Attribute`: it is reset each time the value is less than actual (when a buffer is added, the value is increased) since the final length is not known (no Content-Length) b) `FileUpload`: it is set at startup from the lengh provided So these differences could lead in wrong perception; a) `Attribute`: definedSize = size always b) `FileUpload`: definedSize >= size always Therefore the comment tries to explain clearly the different behaviors. 2) In the InterfaceHttpPostRequestDecoder (and the derived classes), I add a new method: `decoder.currentPartialHttpData()` which will return a `InterfaceHttpData` (if any) as the current `Attribute` or `FileUpload` (the 2 generic types), which will allow then the programmer to check according to the real type (instance of) the 2 methods `definedSize()` and `length()`. This method check if currentFileUpload or currentAttribute are null and returns the one (only one could be not null) that is not null. Note that if this method returns null, it might mean 2 situations: a) the last `HttpData` (whatever attribute or file upload) is already finished and therefore accessible through `next()` b) there is not yet any `HttpData` in decoding (body not yet parsed for instance) Result: The developper has more access and therefore control on the current upload. The coding from developper side could looks like in the example in HttpUloadServerHandler.
fredericBregier
force-pushed
the
master-upload-progress
branch
from
cde2075
to
ab413d4
Compare
|
Note: Jenkins build is ok but returns a bad status for an unrelated reason: |
|
@normanmaurer ping (note Jenkins build was ok, the error is unrelated) |
|
@normanmaurer ping ;-) (and #3663 is for 3.10) |
|
will check tomorrow
|
|
@fredericBregier this looks good but we can only do this in 4.1 and master as this will break the |
|
@netkins build |
|
@normanmaurer Maybe I made something wrong, but I don't think I break the relative interfaces, but adding some new entries in it (not changing previous ones, as in 3.10 too). Of course, classes already implemented outside Netty based on those interfaces will have to build their own. Also without changing the interface might be difficult since one has to be able to add a defined size through factory (which doesn't support it without this extension). One solution could be to add a new setter for Have you other options in mind? |
|
@fredericBregier that's exactly the problem. We can not add new methods to an interface in a bug fix release. |
|
@normanmaurer OK, so if it is strict, I believe there is no patch to apply to 3.10 and 4.0 since we need at least to add the getter/setter for Is that ok to push this only to 4.1/master ? (note that original request from final user seems on master) |
|
Hi @fredericBregier , |
|
@normanmaurer By the way, it is not really a bug fix but a feature + an improvement (since Content-Length was ignored before - improvement - and there was no way to address partial data during upload process - new feature - ). I don't see other ways than adding some methods to Have you an idea how to deal with this rule for this case? |
|
@normanmaurer the jenkins build error seems not related to this PR, right? |
|
@fredericBregier Right. |
|
@netkins build |
|
Note: the build is still failed but not related to this PR. |
|
@netkins build |
|
again failed jenkins build not related to this PR |
|
LGTM. |
Proposal to fix issue #3636 (this has to be fix in all versions)
Motivations:
Currently, while adding the next buffers to the decoder (
decoder.offer()), there is no way to access to the current HTTP object being decoded since it can only be available currently once fully decoded bydecoder.hasNext().Some could want to know the progression on the overall transfer but also per HTTP object.
While overall progression could be done using (if available) the global Content-Length of the request and taking into account each HttpContent size, the per HttpData object progression is unknown.
In addition, currently, the Content-Length is not provided by the client and not taken into account for Attribute.
Modifications:
AbstractHttpDatahas 2 protected properties nameddefinedSizeandsize, respectively the supposely final size and the current (decoded until now) size.This provides a new method
definedSize()to get the current value fordefinedSize. Thesizeattribute is reachable by thelength()method.Note however
definedSizeis only defined if Content-Length is provided (not mandatory). SodefinedSizemight be 0.decoder.currentPartialHttpData()which will return aInterfaceHttpData(if any) as the currentAttributeorFileUpload(the 2 generic types), which will allow then the programmer to checkaccording to the real type (instance of) the 2 methods
definedSize()andlength().This method check if currentFileUpload or currentAttribute are null and returns the one (only one could be not null) that is not null.
Note that if this method returns null, it might mean 2 situations:
a) the last
HttpData(whatever attribute or file upload) is already finished and therefore accessible throughnext()b) there is not yet any
HttpDatain decoding (body not yet parsed for instance)Add Content-Length in default behavior in Multipart Encoder and handling in Multipart Decoder for Attribute (missing parts).
Add constructor for Attribute and Factory to create Attribute if possible with
definedSize.Result:
The developper has more access and therefore control on the current upload.
The coding from developper side could looks like in the example in HttpUloadServerHandler.