% JSON and URL Encoded forms now have the body set #26
Conversation
…ontent-Length Some servers have problems if the Content-Length isn’t set. I don’t see a reason for the body to using streaming when it is a simple encoded form or JSON blob.
There was a problem hiding this comment.
First off, thanks for the PR.
Now, we actually already do this for .formUrlEncoded (look at the top of the method). The reason why it isn't done for JSON is because this transform must happen on the main thread, and we don't want to be encoding arbitrarily-large JSON on the main thread if we don't have to.
Also, if the server requires Content-Length, then surely you'll have this same problem with .multpartMixed, right? Did you just not handle that because you don't care about multipart bodies?
I feel like the right solution here is to add a flag to the request called something like .serverRequiresContentLength (or maybe something like .assumeHTTPOnePointZero, though that's kind of awkward), and then a corresponding .defaultFoo flag on the HTTPManager. Then if this flag is set we do the work on the main thread to ensure we have a Content-Length, and if the flag is not set (the default) we do all this in the background as we're doing today.
Thoughts?
|
Oh great, thanks GitHub for discarding my line comments >_< |
| @@ -1620,6 +1620,10 @@ extension HTTPManager { | |||
| switch uploadBody { | |||
| case .data(let data)?: | |||
| networkTask = inner.session.uploadTask(with: urlRequest, from: data) | |||
| case .formUrlEncoded(let queryItems)?: | |||
| networkTask = inner.session.uploadTask(with: urlRequest, from: FormURLEncoded.data(for: queryItems)) | |||
There was a problem hiding this comment.
This is actually already handled at the top of this method. You'll notice it converts .formUrlEncoded into data up there.
| case .formUrlEncoded(let queryItems)?: | ||
| networkTask = inner.session.uploadTask(with: urlRequest, from: FormURLEncoded.data(for: queryItems)) | ||
| case .json(let json)?: | ||
| networkTask = inner.session.uploadTask(with: urlRequest, from: JSON.encodeAsData(json)) |
There was a problem hiding this comment.
If you want to do this, you need to do it at the top of the method (where .formUrlEncoded is handled), otherwise you're doing a bunch of work inside of a critical section that doesn't need to be done there.
There was a problem hiding this comment.
It appears that NSURLSession doesn't put the Content-Length header in if you're doing streaming. So I don't have a good answer if you don't want to do the JSON encoding on the main thread. The flag seems awkward.
At the moment, I'm only handling JSON uploads.
There was a problem hiding this comment.
What I meant by this is you're encoding JSON inside the inner.sync block, which is a critical section. If you do it at the top of the function, where .formUrlEncoded is handled (i.e. convert the uploadBody to a .data), then you'll get Content-Length (and the cost of encoding JSON on the main thread), it just won't be inside the critical section.
There was a problem hiding this comment.
And yeah, NSURLSession can't possibly put Content-Length in if you're doing streaming, because by definition it has no idea how long the stream is until it's actually streamed the whole thing.
The flag is awkward, but that's ok. The expectation is most servers don't need this (because HTTP 1.1 mandates support for Transfer-Encoding: chunked, so this only applies to HTTP 0.9 or HTTP 1.0 servers).
There was a problem hiding this comment.
Got it; I found this when testing against a Docker instance of our environment; doesn't happen on the real server.
There was a problem hiding this comment.
That's pretty strange. Why wouldn't a Docker instance have the same server behavior as the real thing? Is your server using a different networking library in Docker, for whatever reason?
There was a problem hiding this comment.
I have no idea; I avoid the server stuff!
Thanks for your reply and willingness to put in a "broken" flag.
|
I just implemented the new flag in 3792555. Please let me know if this meets your needs (so I can issue a release that includes it). |
Some servers have problems if the Content-Length isn’t set. I don’t see a reason for the body to using streaming when it is a simple encoded form or JSON blob. If the Content-Length isn't set, the Transfer-Encoding is set as Chunked which causes some problems.
This change is