View previous topic :: View next topic |
Author |
Message |
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Mon Jan 16, 2006 5:21 pm Post subject: HttpClient issues |
|
|
Hello I've been toying around with HttpClient and I've got some suggestions/bug reports:
1. HttpClient uses the default DisplayWriter, which inserts only '\n' to separate lines. AFAICS, "\r\n" should be used. There's an HttpWriter but it's not used by HttpClient and it still uses '\n'.
2. There's more to it in the HttpClient.open function. There are emit() calls with explicit 'CR'. That should be "\r\n" as well.
3. Automatic redirect following should really be optional. I don't really want it in my program and I had to modify Mango
4. This one is the most problematic IMO: there's a test for a 'bogus request' in the HttpClient.open function. It checks if readable == 0. This test is bogus itself It's related to the way reader() works for Sockets: It just reads the first packet of data that recv() returns. The problem is, recv() doesn't return everything at the first call. It's possible, and it's happened to me, that the first received packed contained "HTTP/1.1 200 OK\r\n". What happened ? That line was be skipped, as in the 'skip any blank lines' section. But then input.readable was 0 and the 'bogus request' check triggerred an error. But though the socket may contain more data and not really be bogus If the test is removed, everything seems to work fine.
Ok, that's all for now
Thanks,
Tom |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Mon Jan 16, 2006 7:06 pm Post subject: |
|
|
Thanks, Tom.
* I'm a bit rusty on the HTTP spec, so you're probably right about the \r\n thing.
* How would you prefer to control the redirect? Perhaps through a enableRedirect(bool yes) method? Should it be enabled by default?
* The "truncated response" test is a bit dubious. I think it's actually a hangover from before support for socket-timeout was implemented; but it's been so long I just don't recall the details. There was something about certain sites returning garbage instead of a valid response-line ... hmmm. Perhaps it should be something like this:
Code: |
if (line.get.length == 0)
if (socket.hadTimeout)
.... |
That would catch the situation whereby there was no response, and the following responseLine.parse() will throw if the line is garbage. Does that seem reasonable? |
|
Back to top |
|
|
kylefurlong
Joined: 16 Nov 2005 Posts: 29
|
Posted: Mon Jan 16, 2006 11:53 pm Post subject: |
|
|
Hey Chris!
This class is what I (mainly) would use Mango for. All the things Tom mentioned I have run into as well. Any effort that you could put in on this part of your wonderful library would be appreciated.
Kyle |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Mon Jan 16, 2006 11:58 pm Post subject: |
|
|
kylefurlong wrote: | Hey Chris!
This class is what I (mainly) would use Mango for. All the things Tom mentioned I have run into as well. Any effort that you could put in on this part of your wonderful library would be appreciated.
Kyle |
Certainly; thanks for the support! |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 3:19 am Post subject: |
|
|
kris wrote: | * How would you prefer to control the redirect? Perhaps through a enableRedirect(bool yes) method? Should it be enabled by default? |
That sounds reasonable. I'd say it should be enabled by default.
By the way, I just checked, and redirects work wrong when using the redirectPost method and switching to GET. That's because the headers from the POST method are directly used there, such as Content-Length, Content-Type. In my case, the server gets a non-zero Content-Length and waits for data that never arrives, because it's not a POST request.
Quote: | (...)That would catch the situation whereby there was no response, and the following responseLine.parse() will throw if the line is garbage. Does that seem reasonable? |
Yeah, it works. Just tested.
But of course line.getLength, not line.get.length
Thanks for your support and keep up the good work
Tom |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Tue Jan 17, 2006 11:40 am Post subject: |
|
|
h3r3tic wrote: | kris wrote: | * How would you prefer to control the redirect? Perhaps through a enableRedirect(bool yes) method? Should it be enabled by default? |
That sounds reasonable. I'd say it should be enabled by default.
By the way, I just checked, and redirects work wrong when using the redirectPost method and switching to GET. That's because the headers from the POST method are directly used there, such as Content-Length, Content-Type. In my case, the server gets a non-zero Content-Length and waits for data that never arrives, because it's not a POST request.
Quote: | (...)That would catch the situation whereby there was no response, and the following responseLine.parse() will throw if the line is garbage. Does that seem reasonable? |
Yeah, it works. Just tested.
But of course line.getLength, not line.get.length
Thanks for your support and keep up the good work
Tom |
Thank-you for taking the time on this. I've checked in an update to SVN, which will require a full SVN synch (at least for http, io, text, etc).
Which headers should be removed in that situation? For now, I just cleared all of them except host. BTW, you're welcome to patch these modules if you like (add your name to the file-header) ~ I'm happy to do it and send it back out, but frankly I prefer to share the ownership
(P.S. line.getLength is not valid in SVN head, I recently changed the LineTokenizer into the new Iterator stuff instead). |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 12:15 pm Post subject: |
|
|
kris wrote: | Thank-you for taking the time on this. I've checked in an update to SVN, which will require a full SVN synch (at least for http, io, text, etc). |
No problem. It's open source after all
Quote: | Which headers should be removed in that situation? For now, I just cleared all of them except host. |
In a test I had commenced, both Internet Explorer 6.0 and Firefox 1.5 removed only the Content-Type and Content-Length fields.
Quote: | BTW, you're welcome to patch these modules if you like (add your name to the file-header) ~ I'm happy to do it and send it back out, but frankly I prefer to share the ownership |
I'll try
Quote: | (P.S. line.getLength is not valid in SVN head, I recently changed the LineTokenizer into the new Iterator stuff instead). |
Oh ok. So I shall get everything from SVN |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Tue Jan 17, 2006 12:29 pm Post subject: |
|
|
Ack!
I just thought of a problem we'll run into here ~ MutableHeaders does not currently support removal of individual items. It can be changed to do so, but it'll require an API update. One temporary way around this would be to foreach the existing headers and .dup them into another header-set ... sorry about that. |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 12:34 pm Post subject: |
|
|
kris wrote: | Ack!
I just thought of a problem we'll run into here ~ MutableHeaders does not currently support removal of individual items. It can be changed to do so, but it'll require an API update. One temporary way around this would be to foreach the existing headers and .dup them into another header-set ... sorry about that. |
Yeah, I realize that and I've been planning on doing it thru .dup'ing. But I'll see if adding a remove() is going to be very problematic. Just got mango from svn |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 12:42 pm Post subject: |
|
|
Hey kris, looks like the head revision in svn is incomplete. My program doesn't compile because SimpleIterator.d is missing:
"mango/text/SimpleIterator.d: module SimpleIterator cannot read file 'mango/text/SimpleIterator.d'" |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Tue Jan 17, 2006 1:04 pm Post subject: |
|
|
h3r3tic wrote: | Hey kris, looks like the head revision in svn is incomplete. My program doesn't compile because SimpleIterator.d is missing:
"mango/text/SimpleIterator.d: module SimpleIterator cannot read file 'mango/text/SimpleIterator.d'" |
Fixed |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Tue Jan 17, 2006 1:24 pm Post subject: |
|
|
I added support for removal of a header, via a remove() method ... the HttpToken & TokenStack are a bit touchy, given that their goal is to avoid memory allocation; so I felt I should bear the pain of fixing that part
Now I'll step away, and get back to work. |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 1:26 pm Post subject: |
|
|
kris wrote: | I added support for removal of a header, via a remove() method ... the HttpToken & TokenStack are a bit touchy, given that their goal is to avoid memory allocation; so I felt I should bear the pain of fixing that part
Now I'll step away, and get back to work. |
Thanks So I'll fix the rest in a moment or two /* eating at the moment */ |
|
Back to top |
|
|
h3r3tic
Joined: 30 Mar 2004 Posts: 261 Location: Torun, Poland
|
Posted: Tue Jan 17, 2006 3:55 pm Post subject: |
|
|
Disclaimer: I'm not a HTTP expert myself. I just made it work for my program that used some POSTs and GETs
Here's a diff/patch: http://158.75.59.9/~h3/tmp/httpClientPatch
I've allowed myself to do a few more changes, e.g. modifying the way a query msg is generated - it wasn't separated with '&', then I modified the HttpWriter to put "\r\n" in its endline() func. I made the HttpClient use that writer instead of DisplayWriter.
Another modification: the usage of POST should now be the same as of GET. Earlier, you'd have to manually set the request data, Content-Length and Content-Type. Content-Length will now be set to the query length and Content-Type will be "application/x-www-form-urlencoded".
Hope it works for you
Tom |
|
Back to top |
|
|
kris
Joined: 27 Mar 2004 Posts: 1494 Location: South Pacific
|
Posted: Tue Jan 17, 2006 6:46 pm Post subject: |
|
|
Awesome!
I changed the '&' stuff a little, to make it compatible with tokens.remove() and so on. Also added logic to give 'pump' priority over 'query' when doing a Post ~ I assume these are mutually exclusive?
I do like patches |
|
Back to top |
|
|
|