tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dan Milstein <>
Subject Re: FW: problem w/ ajp13 - if Tomcat is shutdown
Date Mon, 12 Mar 2001 22:20:44 GMT

I looked in the archives and found the patch which I hadn't seen (my ISP's
mail server needs a serious talking to!).

Okay, I think I see some fairly serious issues.  I'll lay out here what I've
seen so far:

 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. 
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?)

 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.

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.  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);

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.

 - 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 ;-).

 - 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, 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. 

 - 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.

 - 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


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:
> For additional commands, email:


Dan Milstein //

To unsubscribe, e-mail:
For additional commands, email:

View raw message