-
Notifications
You must be signed in to change notification settings - Fork 389
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
upload module 2.2.0 fails to build against nginx 1.3.9 #41
Comments
To clarify, this module is ONLY supported up to nginx versions 1.3.8 (dev). up to release-1.2.6 branch is unaffected. and this applies to upload module version 2.2 (https://github.com/vkholodkov/nginx-upload-module/tree/2.2) Background.... Unfortunately, in ngx_http_request.h -> the to_write pointer was removed from the ngx_http_request_body_t structure. Therefore, someone needs to update this code in order to meet up with the new structure and enhancements. |
When planned will be corrected module upload for the nginx 1.3.9 ? |
Hello. I'm also confronted to this issue when testing the upgrade to nginx 1.3.9 As maintainer of the Debian packages for nginx, we might consider dropping you module from the packages if it stays unbuildable for long time. In parallel, I will try to propose a patch for this issue. Thanks. |
It will be nice. |
Apparently, correcting this issue won't be simple as clapping hands. With the 1.3.9 release, the way data are transfered is changed, this lead to changes in the structure ngx_http_request_body_t, used in this module. So this would need to perform some big changes in the modules. I will try to do this, but it will require heavy testing, as I'm not a C developer, and I'm still learning it. Thanks. |
Hello again. I just finished patching this module. For the moment, the only thing I was able to test is the building... and it works. Here is the patch : http://paste.davromaniak.eu/index.php?show=110 I need some people to heavily test it as I'm not an expert in C programming, and it's more a kludge than a clean fix. Thanks. |
Thanks. Currently I switched with your help my test server on nginx 1.3.9 from 1.3.8. |
@davromaniak Thanks for the effort but I have an issue with this patch : Nginx: 413 Request Entity Too Large Error @vkholodkov Do you have any plan to update your code? Thanks |
@benjaminbarbe Thanks for testing, actually, I think my action didn't help advancing on this subject. @vkholodkov Valery, are you still maintaining this module ? Thanks |
@benjaminbarbe thanks for the link, lua-resty-upload looks promising as shows power of Lua in Nginx, but for now it is just an upload stream framework and misses many functions like resumable uploads, crc32, passing control to backend or error handling (correct me if I lost something). You have to implement that logic yourself in Lua. I am sticking with stable ngx 1.2.x now but I see only two ways for future: fork upload-module or make something with that lua stuff (looks easier) - opensource world guys! |
@pgaertig You are right, but look at this example https://gist.github.com/benjaminbarbe/5206051 Scripting Nginx with Lua, useful links: https://twitter.com/benjaminbarbe/status/311827277493182464 |
as I understand nginx-upload-module doesn't work properly with Nginx 1.3.9+? |
Nginx has built-in functionality to provide basic async non-blocking upload without upstream:
|
And after a few evenings here is my attempt: https://github.com/pgaertig/nginx-big-upload . BTW thanks go to @benjaminbarbe for convincing me that Lua is easy. |
@mikhailov you ask for reason to continue support of external modules, and in the same time you say nginx-upload-module is great. Why is it great then if you have My production services process 1GB+ file uploads everyday, resumable is must have in the mobile and WiFi dominated world. For some clients there are non-resumable Flash-based uploads peaking 1GB in one multi-part request. Your solution looks very good but the problem is that multi-part body has to be extracted in 2nd pass by the upstream, which means lag for bigger files. It looks like to me that resumability can be implemented easily using Anyway |
Hi! I feel that I need to comment on this issue. For historical reasons upload module is based on nginx interfaces that are not quite documented. To date there is no incentive from Nginx Inc. to standardize them. Supporting every little change in the core from my side means diving into support hell that doesn't guarantee permanent compatibility either. Doing this means investing a lot of time an effort with questionable outcome. I feel that I did a great job creating all these features and it's time for me to go and find new challenges. I'm pretty sure you will be easily able to fix all compatibility issues. |
@pgaertig You're welcome, thanks to you for nginx-big-upload :-) |
@vkholodkov I dont think the same. Your code is used by so many admins and somany peoples, i cant understand why you brake out to finde new challanges. It was a horror to read it here... now i cant update NGINX because my website will die if i update... thanks for that. :( |
@Dexus why don't you take over the maintenance then? It's not difficult and many people will be grateful to you, including yourself! I'll be always ready to consult you if you need my help... |
@vkholodkov my skills are not so good that i can make it. :( But it looks like i have to learn more about c programming. Am 14.06.2013 um 17:02 schrieb Valery Kholodkov notifications@github.com:
|
Good. You'll get a challenge as well. As I said, I'll always be available for questions. |
This is sad. I'm on the fence about using LUA or another approach for handling uploads. This module was really good and reliable at handling resumable uploads, solid incremental checksums, etc. I'd be interested in sponsoring someone to port this module to work with the nginx development branch (1.5). This upload module saved me many times, especially when having to rely on less-than-ideal app servers (PHP) for uploads. |
@mikhailov - Source? To be more precise, Lua module doesn't break any internals of SPDY module. They both work right in separate requests (production tested), however I had some issues when they are both used in one request. SPDY support is new stuff natively introduced with Nginx 1.4.0 so we have teething time. Production SPDY is still rare outside big G. However if somebody really needs to use both then the workaround is to keep them separated in different |
@mikhailov some of them old, outdated or without evidence. One is even mine, but I assure you currently the Lua module is usable in production if you don't use SPDY. That is OT really. |
I'm afraid you need to complain to Nginx Inc in order to get cooperation on this issue. |
Here is the patch : http://paste.davromaniak.eu/index.php?show=110
----- Original Message -----
|
@ltpao, can you post a complete patch with your bugfix applied? I'm not following your what you're saying. |
Nevermind, I've created a Gist for ngx_http_upload_module.c that has the patch and bugfix applied. I've tested with Nginx v1.5.1 and it seems to be working properly. I'll keep testing and identify any problems on this thread. Much thanks to @davromaniak for creating this patch, @ltpao for finding the bug, and of course @vkholodkov for creating this add-on. |
I've made Ubuntu packages for supported releases from Lucid and going newer On Thu, Sep 5, 2013 at 3:28 PM, Adam Chalemian notifications@github.comwrote:
__ |
Sorry, I should add that my packages are builds of 1.5.4 which is the current newest release as of this writing. http://bit.ly/e6De6N |
@adamchal, thank you for you gist In rare cases nginx terminated with signal 11 nginx/1.4.2 on FreeBSD 9.2-RC3 amd64 here is backtrace:
|
full backtrace:
|
@chrislea > your package seems to not include the upload module :( |
I'm using it right now so I'm pretty sure it does. Did you install the On Thu, Nov 7, 2013 at 3:43 PM, Robin Komiwes notifications@github.comwrote:
__ |
Yep, here is what I'm getting: https://gist.github.com/robink/016c0a9033a09a5212da |
You're using Ubuntu Oneiric, which is no longer supported so the newer On Thu, Nov 7, 2013 at 4:13 PM, Robin Komiwes notifications@github.comwrote:
__ |
The author of |
@mikhailov my system is nginx 1.4.3 +passenger ,I set config according you said:
I still cannot upload file, Could you help me ? |
I used the code on branch 2.2 and succeed in the installation for Nginx 1.4.4. Uploading file also works well. |
Branch 2.2 was successfully installed (and is used) with Nginx 1.5.10. I believe there were great efforts. |
I had issues with the gist in https://gist.github.com/adamchal/6457039 on nginx-1.6.2 due to .state files being incorrectly created. I have modified the code in https://gist.github.com/tchatzi/02dfdf41fafee878cdd5 and it seems to have resolved the issue I was having. |
Given that the original module author (vkholodkov) fixed outstanding issues (apparently) on branch 2.2, then doesn't his work supersede the gist by adam and therefore your fix should be applied to this 2.2 (vkholodkov) branch and not to adamchal as you have done? Doing a code review, your gist is currently missing changes that affect the variable "u" included in branch 2.2. Thanks in advance. |
vkholodkov's master branch still doesn't build against nginx-1.6.2, while mine (basically adamchal's) does and works properly (barring SPDY issues that every module that plays with downstream connections directly seems to have, see openresty/lua-nginx-module#252 and that are a different set of patches anyhow) |
Yeah, would be great to get the patch against 2.2 and not master. |
You're right about version 2.2.0. Barring the known SPDY issues, it seems to work just as well (no thorough code-reading and only light testing done on my part). |
sorry, my bad, I should have explained in more detail:
or worse in adamchal's gist
I couldn't decipher what kind of directories the hashing wanted to pre-exist (in the first case, the second is clearly wrong use of (2) So no, after my simple 3 line patch the .state file problem does not manifest anymore. --- ngx_http_upload_module.c.orig 2014-10-14 21:35:34.024008009 +0300
+++ ngx_http_upload_module.c 2014-10-15 12:14:15.436008728 +0300
@@ -1203,7 +1203,6 @@
ngx_file_t *file = &u->output_file;
ngx_path_t *path = u->store_path;
- ngx_path_t *state_path = u->state_store_path;
uint32_t n;
ngx_uint_t i;
ngx_int_t rc;
@@ -1251,17 +1250,14 @@
return NGX_UPLOAD_NOMEM;
}
- state_file->name.len = state_path->name.len + 1 + state_path->len + u->session_id.len + sizeof(".state")-1;
+ state_file->name.len = file->name.len + 1 + sizeof(".state");
state_file->name.data = ngx_palloc(u->request->pool, state_file->name.len + 1);
if(state_file->name.data == NULL)
return NGX_UPLOAD_NOMEM;
- ngx_memcpy(state_file->name.data, state_path->name.data, state_path->name.len);
- (void) ngx_sprintf(state_file->name.data + state_path->name.len + 1 + state_path->len,
- "%V.state%Z", &u->session_id);
-
- ngx_create_hashed_filename(state_path, state_file->name.data, state_file->name.len);
+ ngx_memcpy(state_file->name.data, file->name.data, file->name.len);
+ ngx_memcpy(state_file->name.data + file->name.len, ".state", sizeof(".state") + 1);
ngx_log_debug1(NGX_LOG_DEBUG_CORE, file->log, 0,
"hashed path of state file: %s", state_file->name.data); (3) In order to replicate the issue, one need only do a chunked http upload, something that requires a valid .state file (4) I did, as I said before, there seems to be a general issue with nginx SPDY and every module that plays with downstream connections directly like openresty/lua-nginx-module#252 quoted above.
and returns an nginx error 500 (5) I'd recommend we fix this, but I'm not (yet ?) knowledgeable enough of nginx internals to figure it out myself. If you can live without SPDY, upload-module works quite as expected. If not, I suppose you'll have to make do with |
@tchatzi thank you very much, this is far better for everyone :) With any luck and if your code is correct, then @vkholodkov may merge. I have reposted the questions that I asked and you answered as follows....
|
I built and ran the 2.2 version of the upload module against NGINX 1.9.4 without any problems. This issue should really be closed or at least reworded. I took it at face value initially and discarded upload-module as a possible solution when, in actuality, it seems to work. |
On a brand new Ubuntu 14.04 server I'm running exactly into this issue when building against nginx 1.9.5:
I'll try to patch this with the solution @tchatzi pointed out. But closing it seems to be a bit too early. :) |
@mikhailov
still not work,can you help me,and i have no back-end,how can i config |
@goodforever @wyzssw I got few questions from you and have an answer here |
@fdintino You can close this issue |
When compiling nginx 1.3.9 and using upload module 2.2.0 as an addon module, the compilation fails (at least for me on Linux using gcc). The error is as follows:
Thanks for your work on this software!
The text was updated successfully, but these errors were encountered: