> 1) First off, I strongly believe that this work (which I think is an
>excellent feature, BTW), belongs in the 3.3 branch, and *not* in 3.2.
+1, there was a message about including latest mod_jk features/corrections
back to Tomcat 3.2.x but TC 3.2 Release Manager and others clairly
stated that TC 3.2.x must be just correction of bugs without any
new features. I asked about back-porting mod_jk/ajp13 since now
Apache 2.0 or upload works fine with mod_jk/ajp from TC 3.3 branches
but are not in TC 3.2. The decision was not to provide corrections
to TC 3.2.
>Although it feels like a bug that we need to restart Apache
>every time we restart TC, it's documented behavior, and I
>consider fixing it more of a feature enhancement.
>The scope of work you're doing here is considerable,
>and the code you're modifying is complex (and involves lots of
>different possible situations). I would not feel comfortable committing
>this to 3.2 unless it had seen a *lot* of testing in the 3.3 branch. (Am
>I incorrect in remembering that there has been discussion of putting this
into 3.2?)
Right many works, some testing but back-port could introduce bugs in TC3.2.
The decision was to not port mod_jk back to tc 3.2. But people could still
build mod_jk from Tomcat 3.3 and use against there Tomcat 3.2.x. That's why
I milite for a jakarta sub-project (web-connector). These kind of connector
work must outside Tomcat trees. (Votes ?)
> 2) Specific problems (line numbers by patch against the 3.3 code base)
>
> - msg/rmsg: Here is how I think you want the code to work:
>msg always holds the basic request information, so, in case of an error, we
can
>resend the message. rmsg is used as the buffer for response information.
Dan you're our resident hacker/expert on mod_jk native part
>This is a good idea, but there are a few problems -- first, and most
>importantly, if there is POST data, the "msg" buffer
>immediately gets reused to send that information to TC (ll. 596-600).
>In that case, on line 691, if you retry after a connection reset event,
>you'd be using a buffer filled with POST data instead of a buffer with
header information.
I'm not too confortable with msg/rmsg that's why I asked for your code
revue (and it's excellent ;-)
>Possibly you could fix this by calling ajp13_marshal_into_msgb again when
>you retry -- I don't know how the state of the web_server_service object
(s),
>changes over time, so I don't know for sure if this would work or not. But
>that's where I'd look next.
>Or you could pass rmsg into the sendrequest, and use that to
>send the POST data over. I dunno.
>Your use of msg/rmsg in getreply seems faulty to me. I don't
>think you need msg there at all, and I'm sure that you've got a problem
with
>mixing the two of them. Specifically, on l. 636, you call
>
> rc = ajp13_process_callback(rmsg, p, s, l);
>
>But, then a few lines later, you do:
>
> else if(JK_AJP13_HAS_RESPONSE == rc) {
> rc = connection_tcp_send_message(p, msg, l);
Right, we must drop rmsg use and do all stuff with msg.
>If you look at ajp13_process_callback, at the GET_BODY_CHUNK
>case, you'll see that it's reading request data into the buffer passed in
as a param
>(which is what getreply is calling 'rmsg'). But then, when you call
>connection_tcp_send_message, you'll be sending over whatever
>was in msg, rather than what the web server has read from the browser.
>You can fix this by changing connection_tcp_send_message to use 'rmsg' (and
>then you don't need to pass msg into getreply at all).
>This problem will happen in case of a file upload, or whenever there is
>enough POST data to exceed a single ajp13 buffer (8K).
>Furthermore, in that
>case, I'm not sure if there's going to be any way to restart
>the connection
>intelligently. If the browser has already sent over 500 K of a 1M file
>upload when TC dies, I think we need to just kill that
>request, rather than
>trying to restart it. I don't think the request is
>recoverable. I'm not sure
>how to detect this in your code, but I think it needs to be
>thought about.
Upload are a real problem since we just couldn't replay the upload
in case of Tomcat failure. Solution could be to ask browser to
resend but HOW (HTTP expert needed here)
> - The for loop on l. 689 makes me nervous. Could you rewrite that:
>int i;
>for(i = 0 ; i < JK_RETRIES ; i++)
>That way you won't get an infinite loop if JK_RETRIES < 0.
>And I think it's
>more idiomatic. But, hey, that's up to you ;-).
+1
> - Do you understand the JK_METHOD macro? I don't fully, but
>take a look in
>jk_global.h. In Win32 and Netware environments, this macro is set to
>"__stdcall" -- is this only needed for things which are called
>from outside
>this file (like service())? If neither of us understand it,
JK_METHOD (stdcall) is needed when used you use functions
from outside but in our case it's static and so it's safe
to not set the JK_METHOD for sendrequest/getreply.
>I'd like to
>have people on Win32 and Netware systems test this (maybe they
>could even
>explain why that call is needed -- and then we could document
>it!) Maybe
>you should be using it with sendrequest and getreply?
> - A few logging inconsistencies -- you call jk_log(l,
>JK_LOG_DEBUG,...),
>when I think you want JK_LOG_ERROR. Check l. 642, 702.
+1,
> - Error/log message inconsistencies:
> - l. 632 You mean "error reading response", I think
> - l. 642 You have "1" where you want ":", I think.
>
> - You seem to be using 4-character tabs -- the rest of the
>C-code (AFAIK) uses 8-character tabs. I think that's our standard
>(it is on the Java side, for sure). In any event, this is
>making your new code render bizarrely in my editor.
I read sometimes ago there was a early decision on TAB length
by Apache team (httpd) but didn't remember what the final decision.
Jon Stevens are to remove all TAB and replace by space, I'll take
a look at native code and set my editor accordinly (vi/ue)
> - Style notes (I'm style obsessive, I admit). I would make it
>ajp13_send_request (or at least send_request) and ajp13_get_reply (or
>get_reply).
Style is important, I'll do that.
I still have problem accessing jakarta-tomcat from CVS in anonymous
mode, but I'll try with my jakarta login. I'll rework the patch
with (or without) rmsg use and resend for study ;-)
Thanks for your analyze.
>-Dan
>
>GOMEZ Henri wrote:
>>
>> >In the mean time, I have taken Henri's changes and back
>> >port it to 3.2.1 (because I need it on 3.2.1). Everything
>> >seems to work well. I've tested it in the normal scenarios
>> >(one Apache, one Tomcat) and in the load-balanced scenarios.
>>
>> Thanks for using the patch and tested it under LoadBalanced
>> Tomcats.
>>
>> >In the load-balanced scenarios, when I restart TC worker 1,
>> >the code properly close the dead sockets and re-establish
>> >new ones to the same worker (TC worker 1). The good
>> >connections to TC worker 2 are untouched. They stay
>> >connected.
>>
>> Normal procedure
>>
>> >I did notice something wierd. But this is un-related
>> >to the code edits. This happens with or without Henri's
>> >changes. When I restart TC worker 1, but shut down TC
>> >worker 2, requests that supposed to go to TC worker 2
>> >(because they belong to the same session, thus the load
>> >balancer try to foward it to the same TC worker 2) took
>> >sometime to get forwarded to TC worker 1. This maybe
>> >another one of those "improvements" that can be done
>> >to the load balancer worker.
>>
>> No problem here, when you shutdown a Tomcat in a
>> load balancing architecture, you got request goes
>> to the second even if there is a JVMROUTE set .
>>
>> >Anyway, I'm pretty happy with Henri's changes. (Thanks
>> >Henri!). Henri, are you going to check in the changes?
>>
>> I'd like to see Dan check it since I created a second memory
>> pool rmsg but I'm not too confident on it :
>>
>> + rmsg = jk_b_new(&(p->pool));
>> + jk_b_set_buffer_size( rmsg, DEF_BUFFER_SZ);
>> + jk_b_reset(rmsg);
>>
>> See you
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>> For additional commands, email: [EMAIL PROTECTED]
>
>--
>
>Dan Milstein // [EMAIL PROTECTED]
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: [EMAIL PROTECTED]
>For additional commands, email: [EMAIL PROTECTED]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, email: [EMAIL PROTECTED]
