#1 Various improvements
To the maintainers of tclhttp1-1
From the few Tcl HTTP/1.1 implementations, I like this
code the best. After a few minor tweaks, I used it
succesfully in my client-server application (server is
tclhttpd).
Because I encountered a few peculiarities, I read the
code, tried to understand it and devised some changes
for the better. Some of them were simple and I
implemented them right away. You find a new version
enclosed/attached.
Some other changes, I did not want to implement
immmediately, but rather ask your comment on my
suggestions. The reason is that I have little
experience with HTTP/1.1 and its RFC. Also, I was a bit
confused by some implementation particulars in the code
and wanted to understand first.
I'm looking forward to your reaction,
Sincerely,
Erik Leunissen
A. Connection type and transaction type
The following is especially relevant at the places in
the code where $state(-command) is used.
the -async option is meant to refer to the *connection*.
the -command option is meant to refer to the *transaction*.
If I'm wrong about this, everything stops right here;
else continue reading ...
Connection type (blocking, async or reused) and
transaction type (async or blocking) can be independent
of each other except for the case of an async connect,
which implies an async transaction by the laws of nature.
Currently, the -async and -command options appear to
interrelate in a non-logical way or their usage is
broken for some combinations. For peculiarities, see
the column `comment' in the table below.
Without knowing what the RFC says about this, the
following table
lists the theoretically possible combinations:
transaction connect release point
type type (async return) comment
-----------------------------------------------------------------
blocking blocking --
reused --
async --
impossible
async blocking after sending
currently broken
reused after sending
works if -async 1 !
async after channel creation
works if -async 1 !
and after sending
Recommendations for implementation:
1.
Strictly separate the *meaning* of the transaction
related -async option from the *meaning* of the
connection related -command option.
2.
Personally, I'm indifferent with respect to the
connection type and I do not care for an option to
control it. I'd prefer code that handles this aspect
without user intervention. I am curious, though, under
what circumstances other programmer have a need to
control this aspect manually.
OK, here's the crude way as I favor it:
- Use a connect type only internally and remove it from
the options;
- Use a transaction type and let the user/programmer
control it through a command line option (like the
current -command option, but its name could be changed
into -async);
- Always connect asynchronously internally.
- If the user specifies a blocking *transaction* and a
new connection is needed, just [vwait] for the
*connection* to complete.
B. Some peculiarities in proc http::writeBodyToChannel:
1.
What was meant with the [write] command in:
if { [catch {set len [write $state(-channel) $b
$sz]} err] } {
Perhaps:
if { [catch {puts -nonewline $state(-channel) $b}
err] } {
2.
Do we need len if we've got sz to indicate the size?
# EOF comment
modified tclhttp1-1.tcl
Logged In: YES
user_id=584339
I'm not sure why I didnt get a note from SourceForge.
tclhttp1-1 was posted on comp.lang.tcl today, so I revisited
the site to see if its changed.
And I found your 4 month old change request :-)
I'll go ahead and look into your notes, and merge if needed.
Phil
Logged In: YES
user_id=113903
Phil,
Two more remarks:
1. In the time between now and my last post, I have made
afew more changes to the code to make it work for a specific
server app. If you can wait until after the coming weekend,
I'll check upon these changes whether I deem them "more
mature than just a tweak", and post these changes also.
2. I see that my previous posting holds a table that looks
very garbled in the plain text sourceforge format. I wonder
whether it is readable if one copies and pastes into an
editor whose maximum character per line is big enough.
Erik.
Logged In: YES
user_id=584339
Your new version would be great.
I can perform the diffs and see whats relavent or not.
I'm also going to patch the mainline http differences for the
http package since 8.3.4 was released.
Logged In: YES
user_id=584339
>1.What was meant with the [write] command in:
>
> if { [catch {set len [write $state(-channel) $b $sz]}
err] } {
>
>Perhaps:
>
> if { [catch {puts -nonewline $state(-channel) $b}
err] } {
>
>2.Do we need len if we've got sz to indicate the size?
1) is indeed a bug
2) if only 100k of a 200k POST is written, we add it, and we
let the eventhandler kick off the function again when more
data arrives.
At least thats what it was supposed to do :-)
I think write was a debugging function I had for a while.
Logged In: YES
user_id=113903
Phil,
Here's the latest version that I'm using to speak to a
tclhttpd server. Mostly cosmetic changes compared to the
previous one. Most notable improvement is the -timeout setup.
Greetings,
Erik Leunissen.
Logged In: YES
user_id=584339
I got the file and will analyze the -timeout feature.
One thing I noticed last week, the tclhttp1-1.tcl code is
MUCH slower than the stock http.tcl.
Youd think it be faster since its reusing the socket.
I'll have to investigate where the slowdown is.
Logged In: YES
user_id=584339
OK, I looked over your code.
1) the timeout thing looks correct. Not sure how that one
got bye.
2) why did you replace http:: with ::http:: ?? Is it a
performance improvement or something ? The standard http
package has http:: so I guess I'll stick with that one for now.
3) using [list] when setting up fileevents and after timers is
also the correct route
4) the main differences seem to be that you dont 'unset the
writable fileevent' in http::reset
switch $why { switch
$why {
ok { ok {
if {[string equal $state(connection) "close"]} {
| if {![string compare $state(connection) "close"]}
>
catch {fileevent $state(sock) writable {}}
catch {fileevent $state(sock) readable
{}} catch {fileevent $state(sock) readable
{}}
http::closeConnection $state
(cid) http::closeConnection $state(cid)
5) Also you dont return anything if the async connect fails to
start up:
# Connect to host
asynchronously # Connect to host
asynchronously
if {[catch { if
{[catch {
eval $state(defcmd) -async $chost
$cport eval $state(defcmd) -async $chost
$cport
} sock]} { | }
sock] {
# Something went wrong while trying to create the
| # Something went wrong while trying to establish
# Clean up after events and such, but DON'T
call # Clean up after events and such, but DON'T
call
# (if available) because we're going to throw
an # (if available) because we're going to throw
an
# instead. #
instead.
http::reset $token error $sock
1 http::reset $token error $sock 1
>
return $token
} }
6) I commited the new version with most of your changes. It
should be version 1.8. Sourceforge takes a while to get the
update.
7) I still need to research why it's so much slower.
I just sent 1000 geturl hits to the same internal web server.
With the stock http package it takes 31 seconds
With this keepalive package it takes 1m 26 seconds.
Logged In: YES
user_id=113903
(Please note that all line numbers refer to the new CVS
version 1.8)
2) why did you replace http:: with ::http:: ?? Is it a
performance improvement or something ? The standard http
package has http:: so I guess I'll stick with that one for now.
>
I don't know whether this affects performance. It's just
that most extensions that come with tcllib, declare their
procs with a leading ::, and I guessed that there is some
reason why extensions generally don't use a relative
namespace. Until I know exactly why, I don't really care
about this a lot though.
4) the main differences seem to be that you dont 'unset the
writable fileevent' in http::reset
>
Not quite.
The line in question is redundant (as it is at line 267)
because the same fileevent is already being deleted at line 236.
5) Also you dont return anything if the async connect fails to
start up:
>
I don't recall the reason. Need to think again why I did that.
6) I commited the new version with most of your changes. It
should be version 1.8. Sourceforge takes a while to get the
update.
>
Thanks for the work.
I see you didn't merge my changes w.r.t. option processing
in http::config (from line 169 onwards, CVS version 1.8) and
http::createrequest (from line 414 onwards). These were
performance improvements because:
- 2 commands less needed
- an [lsearch] instead of [regexp], where lsearch is faster.
(Timing experiments will confirm that).
Don't you agree?
7) I still need to research why it's so much slower.
I just sent 1000 geturl hits to the same internal web server.
With the stock http package it takes 31 seconds
With this keepalive package it takes 1m 26 seconds.
>
I agree that that can't be the intention ;-)
Makes me curious as to the cause.
Logged In: YES
user_id=584339
I kept the regsub option processing stuff as that's the
method done in tcl8.4.7/http.tcl
Eric and myself wanted this library to be incorporated into the
standard TCL distribution, so we tried to make many things
exactly the same.
But from what I'm hearing, this library might be moved to the
tcllib. In which case we can probably replace regsub with
lsearch anyways :-)
Logged In: YES
user_id=113903
>I kept the regsub option processing stuff as that's the
>method done in tcl8.4.7/http.tcl
>
>Eric and myself wanted this library to be incorporated into
the
>standard TCL distribution,
I like that intention, because I think that overall,
http.tcl shows craftmanship w.r.t. its structure at the
extension level and w.r.t. its algorithms. This is a good
reason to adhere to them, but it doesn't mean that changes
to individual code lines don't improve performance.
> so we tried to make many things
>exactly the same.
What reason could anyone have, to withhold improvements from
the "community"? I cannot believe that it requires
restrictions like these to get code included in the standard
TCL distribution.
>But from what I'm hearing, this library might be moved to the
>tcllib. In which case we can probably replace regsub with
>lsearch anyways :-)
How does that depend on the migration to tcllib?