I find this extremely interesting.
Say, If I do the diff on files in driver.tar against the current
CVS HEAD is this OK, In other words, what source did
you take to produce your changes?
Mind providing diff instead of entire files, to be able to review
things easily?
If we agree to get this into, would you take the responsibility
and integrate your changes in current CVS ?
Thanks for your efforts.
Cheer's
Zoran
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The changes agains aolserver4-beta8. I used to do diffs but
to support new versions became kind of hard, so i marked all
my changes in the sources as //Vlad. I use this in our
production so
of course i am responsible for this code, it will be much
easier to support if it would be in the main code.i am
uploading diffs also
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Here it is the patch against 4.0.5. CVS has modified driver
which is not compatible with current stable 4.0.x versions,
i've never checked those modifications.
To test i have modified nssock driver at
ftp://ftp.crystalballinc.com/pub/vlad/nssock.tar.gz
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Very good Vlad.
I will examine this in detail and roll-into if there are no
pitfalls.
Thank you very much for your work.
BTW, the ns_sockopen is now modified to allow for
-localhost and -localport options as per your request. All is in
the CVS head.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
First impressions are good. This change will definitely
be interesting for quite a few folks I believe.
The patch cannot be integrated as-is since the compat-change
for the default nssock driver is missing. It is not
difficult to make
one though.
The example nssock driver you provided is typically what
people would be writing. But the provided example you did
is extremely useful as-is so I think we should integrate this
in the standard distribution and adjust the sample-config.tcl
to mention it. I would not make this one the standard nssock
since it references Tcl which I would not make at that level.
The default driver should be clean of any Tcl constructs.
Not that I do not like Tcl (quite the contrary) but I think we
should have a very clean separation between the core
server and supported languages. The separation now is
frankly speaking miserable but it can (will) be improved as
the time goes.
I'm still not done with the review though. It will take some
more
time but until now it looks promising.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
What is compat-change for nssock?
Why separation from Tcl should be so distinctive? Tcl is in
the core anyway, Tcl interps are allocated for every
connection regardless of any additional languages and Tcl
will always be in the core and not as the language module
which is true for other languages, they will be always as
optional. There is no generic mechanism to define callbacks
which are not Tcl, at least for now.
But the patch itself is not bound to Tcl, it is just an
example uses Tcl so i am not insisting:-)))
Do you need anything from me?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1.
The compat-change? Well, after applying the patch the
nssock driver won't load since it does not recognize
the DriverStart command. Simple.
2.
The separation from Tcl should be gradually done.
Yes, currently, there is very little, thats why I said
"miserable". There is no problem for alternative
nssock to do whatever it wants. Just, I would like
to avoid getting Tcl in all corners of the standard code.
There are already too many of them and we should
strive to make them less, not more.
Now questions:
What I miss in driver.c in DriverThread() is call for the
DriverShutdown command.
Also, I see no call for DriverError. What is the purpose of
that cmd?
The example nssock driver mixes return codes between
NS_OK, NS_FILTER_* and the number of bytes etc.
This is pretty ambigous and should be unified according
to the SockProc() function declaration. Do you see the
chance to correct this?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. Yes, we can make it not that restrictive initially and
ignore start command result, otherwise nssock should be changed.
2. This is new but i guess i might've missed this
development direction,
no big deal, ideally it would be good thing to do if other
languages are going to play big role in the futere.
3. DriverShutdown should be called at the end, looks like i
have missed when porting patch from 4.0.x to 4.1. I may add
it after, so we do not complicate first patch.
4. DriverError was used in 4.0.x when Listen failed and
driver should decide what to do, in my case i did exit what
4.1 does it on Listen failure so may be DriverError is not
needed anymore.
5. This is really an issue. SockProc was designed to do
simpe thing, just retrieve/send data, now it does more
things, it is like driver handler and protocol is more
complicated than before. For now i just used return codes
specific to command called, not good but i did not want to
change drastically the core. Changing SockProc API will
involve changing all comm drivers as nssock, nsopenssl,
nsssl, nsnss. Another way may be to introduce another result
field in the Sock structure and use it to pass results and
keep SockProc returning OK/FAIL only.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
5. For this issue actually i can make in queue.c if SockProc
return NS_OK it means driver took care about this call
otherwise return -1 which means driver does not support it,
so continue as ussual. This will conform to current SockProc
behaviour.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
yes, i just changed in my code return NS_OK and do not use
NS_FILTER_x and it works fine and looks more clean. So i can
send another patch with shutdown added and error removed.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This patch is great considering the context it was created
in: minimally invasive so that it can be maintained outside
the core. But it doesn't integrate well with AOLserver
since the chages from version 3.x to version 4.1.
This patch shortcuts connection processing just after a new
connection is accepted. This means that asyc read-ahead is
not possible, neither is read-ahead by the reader threads,
and neither is the new Ns_PreQueue API.
The problem with the existing structure is that whatever is
read from the socket, the core is hard coded to parse it as
an HTTP request. The solution this patch takes is to
completely shortcut socket reading.
What is really needed are some API hooks for request parsing
(in driver.c: SockRead), or some other means to allow any
new protocol driver to take advantage of all the existing
read/queue structure.
Consider a new protocol driver that needs to also implement
SSL/TLS encryption. It will have to completely reimplement
nsopenssl.
The only reason this is a problem is because the patch adds
a new API rather than a new implementation, and once it is
accepted it will be difficult to change later. It would be
a shame to come this far and end up locked into an API less
powerful than it could be.
What do you think about extending the API so that it's
possible to take advantage of the AOLserver infrastructure?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
That could be possible in aolserver 3.x, more difficult in
4.0 and almost impossible in 4.1. It is more HTTP oriented
than i'd like it to be but still main purpose is to server
HTTP with some optional drivers. Even with this patch it
will reuse connection pooling and limits, but not pre-queue
and read-ahead, but again, those features were designed for
HTTP protocol, new drivers can implement similar thing if
needed, it is not that much of the code. Currenlty i am fine
with non-invasive approach and i do not think non-http
protocols will need those features too much. At least in my
SMTP driver i do not need read-ahead and i implemented SMTP
related things inside the driver, thet are not related to HTTP.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Frankly, I see shoe-horning support for non-HTTP traffic into
nssock as a mistake. Perhaps "nssock" should be
renamed "nshttp".
The right answer here is to write a new driver thread and
code and creating a new socket driver module which could be
used as a generic TCP service handler. We could easily bring
back nsftp (AOLserver as an FTP server) this way, etc.
Having a generic TCP service handler that lets you write the
server code in pure Tcl would make prototyping and initial
development of assorted TCP services (FTP, SMTP, etc.) very
quick and easy, and once the right thread model, connection
queueing model, and worker thread allocation method is
established, re-implementing the performance-sensitive driver
thread in C may be worthwhile for sites that need the stuff to
really scale.
Can we keep the current "nssock" module (perhaps better
renamed to "nshttp") tuned and optimized for handling HTTP
connections, as the characteristics of such service really
dictate the design decisions you see in the current nssock
module? HTTP is typically a very high connection rate service
with very short-lived TCP connections exchanging a low
number of TCP packets per request. This is quite different to
FTP (long lasting connections, where the data connection
typically exchanges many packets at high speed) and SMTP
(medium-length connection life, back-and-forth exchange of
small packets of data, then a stream of data followed by
disconnect).
I don't know if I'm adequately getting my point across, here.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think we all agree that first and foremost AOLserver is a
webserver, and that stability, performance and clean code
are the main concerns. I don't think these requirements are
at odds with providing multi protocol support.
Please take a look at the patch I added to #973010.
It intersects the original code at only two points:
SockRead in driver.c has been split into SockRead and
SockParse, where SockRead handles only reading, and if no
Ns_ParseProc was registerd, calls SockParse.
NsConnThread in queue.c checks for an Ns_RunProc, and if
none has been registered, calls ConnRun as before.
None of the above checking requires any locks.
Does this look at all promising?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Logged In: YES
user_id=184124
header file
Logged In: YES
user_id=184124
queue/connection updates
Logged In: YES
user_id=184124
deleting extra ns.h
Logged In: YES
user_id=184124
Improved changes to base aolserver4 driver support,
driver.tar is stable version.
Logged In: YES
user_id=95086
I find this extremely interesting.
Say, If I do the diff on files in driver.tar against the current
CVS HEAD is this OK, In other words, what source did
you take to produce your changes?
Mind providing diff instead of entire files, to be able to review
things easily?
If we agree to get this into, would you take the responsibility
and integrate your changes in current CVS ?
Thanks for your efforts.
Cheer's
Zoran
Logged In: YES
user_id=184124
Sorry for not providing the details.
The changes agains aolserver4-beta8. I used to do diffs but
to support new versions became kind of hard, so i marked all
my changes in the sources as //Vlad. I use this in our
production so
of course i am responsible for this code, it will be much
easier to support if it would be in the main code.i am
uploading diffs also
Logged In: YES
user_id=95086
Hi Vlad,
I think the patch looks good and I will integrate it in the CVS
head. Do you have:
* example driver that utilizes the new code so I can check
that
* diff-files for the current CVS head state of AS
I suppose you do, since your're using this in your env, right?
Zoran
Logged In: YES
user_id=184124
Here it is the patch against 4.0.5. CVS has modified driver
which is not compatible with current stable 4.0.x versions,
i've never checked those modifications.
To test i have modified nssock driver at
ftp://ftp.crystalballinc.com/pub/vlad/nssock.tar.gz
Logged In: YES
user_id=184124
Here is the patch(driver41.diff) against current CVS HEAD
version. It is even simplier than for pervious 4.0.x
versions, works the same way.
Logged In: YES
user_id=95086
Very good Vlad.
I will examine this in detail and roll-into if there are no
pitfalls.
Thank you very much for your work.
BTW, the ns_sockopen is now modified to allow for
-localhost and -localport options as per your request. All is in
the CVS head.
Logged In: YES
user_id=95086
First impressions are good. This change will definitely
be interesting for quite a few folks I believe.
The patch cannot be integrated as-is since the compat-change
for the default nssock driver is missing. It is not
difficult to make
one though.
The example nssock driver you provided is typically what
people would be writing. But the provided example you did
is extremely useful as-is so I think we should integrate this
in the standard distribution and adjust the sample-config.tcl
to mention it. I would not make this one the standard nssock
since it references Tcl which I would not make at that level.
The default driver should be clean of any Tcl constructs.
Not that I do not like Tcl (quite the contrary) but I think we
should have a very clean separation between the core
server and supported languages. The separation now is
frankly speaking miserable but it can (will) be improved as
the time goes.
I'm still not done with the review though. It will take some
more
time but until now it looks promising.
Logged In: YES
user_id=184124
What is compat-change for nssock?
Why separation from Tcl should be so distinctive? Tcl is in
the core anyway, Tcl interps are allocated for every
connection regardless of any additional languages and Tcl
will always be in the core and not as the language module
which is true for other languages, they will be always as
optional. There is no generic mechanism to define callbacks
which are not Tcl, at least for now.
But the patch itself is not bound to Tcl, it is just an
example uses Tcl so i am not insisting:-)))
Do you need anything from me?
Logged In: YES
user_id=95086
First, answers:
1.
The compat-change? Well, after applying the patch the
nssock driver won't load since it does not recognize
the DriverStart command. Simple.
2.
The separation from Tcl should be gradually done.
Yes, currently, there is very little, thats why I said
"miserable". There is no problem for alternative
nssock to do whatever it wants. Just, I would like
to avoid getting Tcl in all corners of the standard code.
There are already too many of them and we should
strive to make them less, not more.
Now questions:
What I miss in driver.c in DriverThread() is call for the
DriverShutdown command.
Also, I see no call for DriverError. What is the purpose of
that cmd?
The example nssock driver mixes return codes between
NS_OK, NS_FILTER_* and the number of bytes etc.
This is pretty ambigous and should be unified according
to the SockProc() function declaration. Do you see the
chance to correct this?
Logged In: YES
user_id=184124
1. Yes, we can make it not that restrictive initially and
ignore start command result, otherwise nssock should be changed.
2. This is new but i guess i might've missed this
development direction,
no big deal, ideally it would be good thing to do if other
languages are going to play big role in the futere.
3. DriverShutdown should be called at the end, looks like i
have missed when porting patch from 4.0.x to 4.1. I may add
it after, so we do not complicate first patch.
4. DriverError was used in 4.0.x when Listen failed and
driver should decide what to do, in my case i did exit what
4.1 does it on Listen failure so may be DriverError is not
needed anymore.
5. This is really an issue. SockProc was designed to do
simpe thing, just retrieve/send data, now it does more
things, it is like driver handler and protocol is more
complicated than before. For now i just used return codes
specific to command called, not good but i did not want to
change drastically the core. Changing SockProc API will
involve changing all comm drivers as nssock, nsopenssl,
nsssl, nsnss. Another way may be to introduce another result
field in the Sock structure and use it to pass results and
keep SockProc returning OK/FAIL only.
Logged In: YES
user_id=184124
5. For this issue actually i can make in queue.c if SockProc
return NS_OK it means driver took care about this call
otherwise return -1 which means driver does not support it,
so continue as ussual. This will conform to current SockProc
behaviour.
Logged In: YES
user_id=184124
yes, i just changed in my code return NS_OK and do not use
NS_FILTER_x and it works fine and looks more clean. So i can
send another patch with shutdown added and error removed.
Logged In: YES
user_id=87254
This patch is great considering the context it was created
in: minimally invasive so that it can be maintained outside
the core. But it doesn't integrate well with AOLserver
since the chages from version 3.x to version 4.1.
This patch shortcuts connection processing just after a new
connection is accepted. This means that asyc read-ahead is
not possible, neither is read-ahead by the reader threads,
and neither is the new Ns_PreQueue API.
The problem with the existing structure is that whatever is
read from the socket, the core is hard coded to parse it as
an HTTP request. The solution this patch takes is to
completely shortcut socket reading.
What is really needed are some API hooks for request parsing
(in driver.c: SockRead), or some other means to allow any
new protocol driver to take advantage of all the existing
read/queue structure.
Consider a new protocol driver that needs to also implement
SSL/TLS encryption. It will have to completely reimplement
nsopenssl.
The only reason this is a problem is because the patch adds
a new API rather than a new implementation, and once it is
accepted it will be difficult to change later. It would be
a shame to come this far and end up locked into an API less
powerful than it could be.
What do you think about extending the API so that it's
possible to take advantage of the AOLserver infrastructure?
Logged In: YES
user_id=184124
That could be possible in aolserver 3.x, more difficult in
4.0 and almost impossible in 4.1. It is more HTTP oriented
than i'd like it to be but still main purpose is to server
HTTP with some optional drivers. Even with this patch it
will reuse connection pooling and limits, but not pre-queue
and read-ahead, but again, those features were designed for
HTTP protocol, new drivers can implement similar thing if
needed, it is not that much of the code. Currenlty i am fine
with non-invasive approach and i do not think non-http
protocols will need those features too much. At least in my
SMTP driver i do not need read-ahead and i implemented SMTP
related things inside the driver, thet are not related to HTTP.
Logged In: YES
user_id=21885
Frankly, I see shoe-horning support for non-HTTP traffic into
nssock as a mistake. Perhaps "nssock" should be
renamed "nshttp".
The right answer here is to write a new driver thread and
code and creating a new socket driver module which could be
used as a generic TCP service handler. We could easily bring
back nsftp (AOLserver as an FTP server) this way, etc.
Having a generic TCP service handler that lets you write the
server code in pure Tcl would make prototyping and initial
development of assorted TCP services (FTP, SMTP, etc.) very
quick and easy, and once the right thread model, connection
queueing model, and worker thread allocation method is
established, re-implementing the performance-sensitive driver
thread in C may be worthwhile for sites that need the stuff to
really scale.
Can we keep the current "nssock" module (perhaps better
renamed to "nshttp") tuned and optimized for handling HTTP
connections, as the characteristics of such service really
dictate the design decisions you see in the current nssock
module? HTTP is typically a very high connection rate service
with very short-lived TCP connections exchanging a low
number of TCP packets per request. This is quite different to
FTP (long lasting connections, where the data connection
typically exchanges many packets at high speed) and SMTP
(medium-length connection life, back-and-forth exchange of
small packets of data, then a stream of data followed by
disconnect).
I don't know if I'm adequately getting my point across, here.
Logged In: YES
user_id=184124
I would rename generic nssock example into nstcp and leave
nssock as is.
Logged In: YES
user_id=95086
Vlad,
The new patch file is broken?
zoran@zvlinux:~/sf/aolserver-head/nsd> patch -p0 <
driver-4.1.diff
patching file driver.c
patch: **** unexpected end of hunk at line 38
I'm applying it against 4.1 head. Do I miss something?
Logged In: YES
user_id=87254
I think we all agree that first and foremost AOLserver is a
webserver, and that stability, performance and clean code
are the main concerns. I don't think these requirements are
at odds with providing multi protocol support.
Please take a look at the patch I added to #973010.
It intersects the original code at only two points:
SockRead in driver.c has been split into SockRead and
SockParse, where SockRead handles only reading, and if no
Ns_ParseProc was registerd, calls SockParse.
NsConnThread in queue.c checks for an Ns_RunProc, and if
none has been registered, calls ConnRun as before.
None of the above checking requires any locks.
Does this look at all promising?
Patch for AOLserver 4.1 driver.c (Modified)