Mailing-List: contact tomcat-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
Reply-To: tomcat-dev@jakarta.apache.org
Message-ID: <361024C34A6DD2118689006097AE2B4D0102C79D@css4.cs>
From: GOMEZ Henri <hgomez@slib.fr>
To: tomcat-dev@jakarta.apache.org
Cc: danmil@shore.net
Subject: RE: FW: problem w/ ajp13 - if Tomcat is shutdown
Date: Tue, 13 Mar 2001 23:48:53 +0100
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"

> 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: tomcat-dev-unsubscribe@jakarta.apache.org
>> For additional commands, email: tomcat-dev-help@jakarta.apache.org
>
>-- 
>
>Dan Milstein // danmil@shore.net
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
>For additional commands, email: tomcat-dev-help@jakarta.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
For additional commands, email: tomcat-dev-help@jakarta.apache.org