412 Pre-Condition Failed returns after whole body uploaded #8
Comments
gencer
changed the title
|
The This mean you're either using a web server which buffers the request body before forwarding it to the Rack application ( Puma buffers the request body, while Goliath, Unicorn, and I think Passenger don't. If you're mounting the If you want the request body to be buffered, you should run the tus server as a standalone app on Goliath, as explained in the README. Goliath is recommended over Unicorn or Passenger because it's vulnerable to slow clients, meaning the client's upload/download speed doesn't affect the overall request throughput. |
|
I agree that its processed as soon as headers received, However, my nginx configuration has request and response buffering off and I already use Goliath. Still have the same issue. Also, Let's say I start from an invalid offset; 409 Conflict error came after whole body uploaded. But the thing is if upload already finished I get that error before file uploaded. I will investigate more and update here. I'm sure something is broken in somewhere (I still don't know yet) |
|
Thanks for providing more details. That is strange indeed then. The
Hmm, that means that streaming does work, so there might indeed be a bug somewhere. I would suggest you try to override the class DebugRead
def initialize(app)
@app = app
end
def call(env)
env["rack.input"].instance_eval { def read(*); raise "#read is called"; end }
@app.call(env)
end
end
Tus::Server.use DebugRead |
It's getting interesting. This traceback from fresh file being uploaded. When 412 error happens there is no read. However, it is still upload the whole file. What I did; I stopped TusServer (Ruby) and start the Tusd (GoLang). I sent the same request and 412 given before file uploaded. Same environment but different languages. I'm suspecting that TusServer and/or Goliath doesn't intercept request until all received. It should break the connection somehow and return the error. That's what tusd does. |
|
Thanks a lot for checking that. I think the I found the issue: The goliath-rack_proxy gem, the glue between Goliath and the I recall thinking about this when I was writing goliath-rack_proxy, but I don't remember what conclusion I reached back then. I'll have another look. |
|
I'm more than happy to help you on this case. I was eating my brain since few hours :P BTW;
|
|
Thanks, I'm currently reading the Goliath source code to see how I can return a response early. The most important code is in goliath/api.rb and goliath/connection.rb, feel free to take a look if you want 😃 |
In this case I would recommend going with The Ruby implementation is useful mostly because you can mount it as a Rack endpoint into the existing app, i.e. you don't need to run it as a separate process. I also wanted to prove that it's possible to write something advanced like this in Ruby, with streaming and everything, and I think it has good performance (I was benchmarking it against But it will never have such a big userbase as |
I am looking at it and also to Goliath's all source code to understand whats + and -.
You'll never know. Maybe this will get more attention. However, For |
|
Ok, I think I fixed it on the goliath-rack_proxy Btw, I'm just curious, doesn't this issue affect only the scenario where the client sent out the wrong request, which wouldn't be something that happens in production?
Thanks a lot, that's really nice to hear ❤️ |
You mean to say that evented code is difficult to wrap your head around? 😜 I feel you, there was a lot of head banging on my side during writing goliath-rack_proxy, as I've never used EventMachine or Goliath before that. It's probably one of the most difficult codebases I had to navigate through (though I think the Rails codebase was still harder). But after a lot of looking and reading EventMachine's API documentation you do get a sense of how it all fits together 😃 |
Yes, It is only that If client sends an invalid request. However, Imagine that even in production, client sends a request wrongly and added 10GB file and imagine after we say 412 after grabbing whole file. That is still a probable. And after that error I wouldn't dare to face with that client 😜 (Especially if we consider that client has a slow connection)
I'm always a little bit scared of them. (especially anything that's single threaded async i/o)
I still feel its complicated 😜
Something is preventing me to fetch from your master branch. |
You're right, it probably can happen, and yeah, in this case it's better to fail early. I was just asking because I'm always wary of making changes related to Goliath, because I feel like I'm playing with fire 🔥, but I think this change should be ok.
Hmm, I'm not getting this error, this is what I'm using: gem "goliath-rack_proxy", github: "janko-m/goliath-rack_proxy"I'm using the same version of Ruby and Bundler as you, so I really don't know what gives. The only thing I could tell from the stack trace is that Bundler tried to print an error message to the console, but it contained invalid characters. I have no idea why, I don't think it's related to the recent changes. |
Nope, It wasn't. Updating bundler solved the problem. (I was very sleepy at that moment couldn't think of updating it...) Sorry for the false alarm. However, It's still does not intercept my request but await for the whole body finish. I get 412 or 415 errors after file gets uploaded. |
|
Ok, I realized why it's not working. Goliath always waits for the request body to be received before writing the response data, I opened a ticket for that – postrank-labs/goliath#348. For now I'll add a workaround for it in goliath-rack_proxy. I'll make it opt-in, because not all HTTP clients support early responses (e.g. http.rb doesn't at the moment). |
|
Actually, scratch that. If a HTTP client doesn't support early responses, then that client ought to be fixed. If tusd returns early responses, then so should tus-ruby-server. |
|
Ok, I pushed another commit to goliath-rack_proxy which should this time correctly send responses early, and learned a few more things about TCP sockets along the way. Let me know how it works for you. |
|
False alarm. Discard my previous reply (deleted). It just worked. It intercepts and returns with 412 error successfully. 👍 |
|
I released goliath-rack_proxy 1.1.0 with the latest changes. |
At current state, we upload (PATCH) a chunk or whole file. After that if headers are wrong or missing, 412 error returned. However, we should let client before we receive the file.
Currently, the whole body file uploaded regardless it has invalid or missing headers.
How we should handle errors before we accept file?
The text was updated successfully, but these errors were encountered: