Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sometimes it's usefull to watch several simultaneous uploads in one request. Especially for a big number of such uploads. #30

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

devgs
Copy link

@devgs devgs commented Jun 18, 2012

Sorry if it looks hacky for you.

@thomascrown
Copy link

How would I call this in a form and what javascript would I use to get multiple status for a particular unique ID? Do I have to give each upload a unique ID? Do you have an example of the config file you used? This looks exciting if I could get it working!!!

@devgs
Copy link
Author

devgs commented Jul 12, 2012

How would I call this in a form and what javascript would I use to get multiple status for a particular unique ID?

Nope, you perform several uploads simultaneously when each has unique ID.

Do I have to give each upload a unique ID?

Yes.

Do you have an example of the config file you used?

Nothing special

http {

upload_progress proxied 1m;

server {

   listen 127.0.0.1:80;
   location /progress {
      report_uploads proxied;
      upload_progress_json_multiple_output;
   }

   location ~ /upload/.* {
                access_log  /var/log/nginx/access.log main;
                proxy_pass  http://file-backend;

                proxy_redirect     off;

                proxy_set_header   Host             $host;
                proxy_set_header   X-Real-IP        $remote_addr;
                proxy_set_header   X-Forwarded-For  $proxy_add_x_forwarded_for;
                proxy_intercept_errors on;

                client_max_body_size       0;
                client_body_buffer_size    512k;

                client_body_temp_path      /tmp/nginx/client_body_temp;

                proxy_connect_timeout      120s;
                proxy_send_timeout         120s;
                proxy_read_timeout         120s;

                proxy_buffer_size          256k;
                proxy_buffers              128 4k;
                proxy_busy_buffers_size    256k;
                proxy_temp_file_write_size 256k;

                track_uploads proxied 30s;
        }
}

This looks exciting if I could get it working!!!

To get upload status you knock /progress with header X-ProgressMultiple-ID set to <ID1>[;<ID2>[;<ID3>...]]

And you will get JSON array

[<json_multiple_pattern for ID1>, <json_multiple_pattern for ID1>, ...]

Default pattern is :

static ngx_str_t ngx_http_uploadprogress_json_multiple_defaults[] = {
    ngx_string("{ \"id\" : $uploadprogress_id, \"state\" : \"starting\" }"),
    ngx_string("{ \"id\" : $uploadprogress_id, \"state\" : \"error\", \"status\" : $uploadprogress_status }"),
    ngx_string("{ \"id\" : $uploadprogress_id, \"state\" : \"done\" }"),
    ngx_string("{ \"id\" : $uploadprogress_id, \"state\" : \"uploading\", \"received\" : $uploadprogress_received, \"size\" : $uploadprogress_length }")
’’’
};

@thomascrown
Copy link

@devgs Having a problem compiling your patch - keep getting build errors when running *make
in function 'ngx_http_track_uploads
1723: error: 'NGX_PARSE_LARGE_TIME' undeclared
1723: error: (Each undeclared identifier is reported only once for each function it appears in

@devgs
Copy link
Author

devgs commented Jul 12, 2012

@thomascrown
Mhm... I can't find (NGX_PARSE_LARGE_TIME) symbol there. Also line 1723 is not in the function ngx_http_track_uploads.

Maybe you are trying to compile different version of source code?

@thomascrown
Copy link

@devgs Yup - it was my bad - no idea what file I was trying to use - i just replaced the .c file with your changed version. It seams to be working - thanks for the help so far - I really appreciated it!

Now I have other problems:
I can call the progress OK with X-ProgressMultiple-ID.... but it always says starting. My setup worked without the patch & with just one file using the jquery.uploadProgress.js extension....

Do you have a working example of a form with javascript that has multiple uploads working?

My config for Nginx server level is as follows:

location @frontcontroller {
rewrite ^ /upload/index.php last;
}

location = /progress {
report_uploads uploads;
upload_progress_json_multiple_output;
}

location /upload {
upload_pass @frontcontroller;
upload_store /var/www/tmp_upload 1;
upload_store_access user:r group:r all:r;
upload_set_form_field $upload_field_name.name "$upload_file_name";
upload_set_form_field $upload_field_name.path "$upload_tmp_path";
upload_aggregate_form_field "$upload_field_name.sha1" "$upload_file_sha1";
upload_aggregate_form_field "$upload_field_name.size" "$upload_file_size";
upload_pass_form_field "^target$";
upload_cleanup 400 404 499 500-505;
track_uploads uploads 5s;
}

I cannot thank you enough for your help!!!

@SigSegFault
Copy link

@thomascrown That's me (devgs). Sorry for posting from different account.

From what i read in your doc:
''' Since Nginx doesn't support yet RFC 1867 upload, the location must be a proxy_pass or fastcgi location.'''

upload_pass will not work. It says 'starting', because when there is no such ID found, you return such status.

I hope that will help.

So far i can't provide example because i am back-end developer and have poor knowledge of JS :(

But soon we are going to use this in production and if you wish, I can give link you our website so you can test it with firebug.

@thomascrown
Copy link

@devgs @SigSegFault Thanks for the help - I do have the front end working - and the setup that I pasted above works when I am using the non patched version of progress upload...

In any case - I created a wrapper that accept dropped files, and that passes each file to the upload processor with a unique ID for each... it does the trick and Im getting accurate load times.

Once again - many thanks for the help!

@devgs
Copy link
Author

devgs commented Jul 13, 2012

Happy to help ;) Thank you.

@masterzen
Copy link
Owner

I know this is a long time with no answers on this PR, but can you please squash those commit in a more coherent patch set.

Oh, and also move out the graphite commit in its own PR out of this one would be really good.

Also it would be great to support the multiple ids with the exact same API as the single one (ie send the same headers, but detect when more than one id is sent and thus return a json array instead of a single json thing).

Thanks,

msva pushed a commit to msva/nginx-upload-progress-module that referenced this pull request May 28, 2019
Signed-off-by: Vadim A. Misbakh-Soloviov <git@mva.name>
msva pushed a commit to msva/nginx-upload-progress-module that referenced this pull request Aug 14, 2022
Signed-off-by: Vadim A. Misbakh-Soloviov <git@mva.name>
@BenBE
Copy link
Contributor

BenBE commented Dec 8, 2024

@masterzen While looking into the open PRs for this repo when I was updating the modules for my local nginx build today, I also did a cleanup of this PR. You can find the result of this work in my repo on branch multi-uploads. This does not yet eliminate the UDP extension you asked for to be removed from this PR, but if you want I can drop that commit on my branch easily.

My branch does not include any changes to support multiple IDs via the same API as you suggested in your most recent comment here either, but I think this should be doable with little extra effort. If you want I can take a look.

If both @masterzen and @devgs agree, I can split my branch into three in order to supersede this PR.

Please note, that my branch contains a major source code formatting patch in front of the actual work as the current master in this repository is quite inconsistent and the used formatting was all over the place. Using a more consistent formatting makes porting patches much more workable as there's a defined way to do things™ … This formatting is not fully perfect either, but a minimum step I deem necessary to simplify the further maintenance of the code base.

Speaking of maintenance: @masterzen, would you agree to place this repo into a GH organization account and share (commit) access? The other option would be for me to hard-fork this repo in the long-run when integrating PRs that aren't yet merged here. This both duplicates work unnecessarily and also may cause confusion where active development takes place if there are multiple diverging repositories with ongoing changes.

@masterzen
Copy link
Owner

@masterzen While looking into the open PRs for this repo when I was updating the modules for my local nginx build today, I also did a cleanup of this PR. You can find the result of this work in my repo on branch multi-uploads. This does not yet eliminate the UDP extension you asked for to be removed from this PR, but if you want I can drop that commit on my branch easily.

Would it be possible to open a PR here?
That's going to be simpler for reviewing and merging.

I'm still not convinced the UDP extension is something that needs to be part of that project :)

My branch does not include any changes to support multiple IDs via the same API as you suggested in your most recent comment here either, but I think this should be doable with little extra effort. If you want I can take a look.

I would say that we don't necessarily need it, but it's more a nice to have. If you want to spend a bit of time providing this, feel free to do so.

If both @masterzen and @devgs agree, I can split my branch into three in order to supersede this PR.

Yes, I do agree. You kept @devgs contributions, so I think you can do this even if we never get an answer from them.

Please note, that my branch contains a major source code formatting patch in front of the actual work as the current master in this repository is quite inconsistent and the used formatting was all over the place. Using a more consistent formatting makes porting patches much more workable as there's a defined way to do things™ … This formatting is not fully perfect either, but a minimum step I deem necessary to simplify the further maintenance of the code base.

Agreed. What did you use to format the code?
We could have a format definition alongside this project for all modifications to follow it, even using some github actions for checking format is correct?

Speaking of maintenance: @masterzen, would you agree to place this repo into a GH organization account and share (commit) access? The other option would be for me to hard-fork this repo in the long-run when integrating PRs that aren't yet merged here. This both duplicates work unnecessarily and also may cause confusion where active development takes place if there are multiple diverging repositories with ongoing changes.

As you have noticed I'm not actively developing this project anymore as I don't need it anymore. Moving to an organization is one way of doing it, but I don't ever want to finish in a situation like the XZ project :(
My spare time for this project is very small nowadays as I moved to other things in the past 15 years :), but I can still do reviews for merging PRs.

Can we first have this PR merged, and if you have more to merge let me know and I'll review merge accordingly. Then we can revisit the organization creation.
Would that work for you @BenBE?

@BenBE
Copy link
Contributor

BenBE commented Dec 8, 2024

@masterzen While looking into the open PRs for this repo when I was updating the modules for my local nginx build today, I also did a cleanup of this PR. You can find the result of this work in my repo on branch multi-uploads. This does not yet eliminate the UDP extension you asked for to be removed from this PR, but if you want I can drop that commit on my branch easily.

Would it be possible to open a PR here? That's going to be simpler for reviewing and merging.

Sure, just wanted to confirm with @devgs first, as it's basically their code.

I'm still not convinced the UDP extension is something that needs to be part of that project :)

Me neither. I'd also rather like to drop it and will do so for the PR.

(@devgs : There will be a branch with the rebased changes for the UDP stuff too; I'll just wont update it further).

My branch does not include any changes to support multiple IDs via the same API as you suggested in your most recent comment here either, but I think this should be doable with little extra effort. If you want I can take a look.

I would say that we don't necessarily need it, but it's more a nice to have. If you want to spend a bit of time providing this, feel free to do so.

While looking through the source I noticed quite a few places that seem very repeated to me. And having just one API implemented is for sure a way to reduce code duplication (aside from some refactoring I'll try to get in later).

If both @masterzen and @devgs agree, I can split my branch into three in order to supersede this PR.

Yes, I do agree. You kept @devgs contributions, so I think you can do this even if we never get an answer from them.

Will likely do some minor Git magic too to make ensure proper attribution.

Please note, that my branch contains a major source code formatting patch in front of the actual work as the current master in this repository is quite inconsistent and the used formatting was all over the place. Using a more consistent formatting makes porting patches much more workable as there's a defined way to do things™ … This formatting is not fully perfect either, but a minimum step I deem necessary to simplify the further maintenance of the code base.

Agreed. What did you use to format the code? We could have a format definition alongside this project for all modifications to follow it, even using some github actions for checking format is correct?

The formatting was done by hand using the code style I usually follow. It's roughly based on the one from htop (indentation kept at 4 spaces ;-)), but as that one was mostly a first step I tried to keep it at a minimum. The htop guide also mostly gives a good guidance, instead of being strict. You can somewhat use astyle for formatting, but final judgement should be done as necessary and where changes actully were to reduce noise.

Speaking of maintenance: @masterzen, would you agree to place this repo into a GH organization account and share (commit) access? The other option would be for me to hard-fork this repo in the long-run when integrating PRs that aren't yet merged here. This both duplicates work unnecessarily and also may cause confusion where active development takes place if there are multiple diverging repositories with ongoing changes.

As you have noticed I'm not actively developing this project anymore as I don't need it anymore. Moving to an organization is one way of doing it, but I don't ever want to finish in a situation like the XZ project :( My spare time for this project is very small nowadays as I moved to other things in the past 15 years :), but I can still do reviews for merging PRs.

Understandable. Unfortunately that's always a risk with (unpaid) volunteer work.

I'm running a (custom built) nginx on my infra with this and a few other modules added. Thus seeing these modules properly maintained is quite important.

Can we first have this PR merged, and if you have more to merge let me know and I'll review merge accordingly. Then we can revisit the organization creation. Would that work for you @BenBE?

Sure. Just wanted to bring this up as an option. No need to rush.

@devgs
Copy link
Author

devgs commented Dec 9, 2024

@BenBE Go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants