On 1/27/09 1:46 PM, Ingo Strüwing wrote:
> Hi Davi,
> Davi Arnaut, 26.01.2009 19:30:
>> On 1/21/09 3:05 PM, Ingo Strüwing wrote:
>>> Davi Arnaut, 20.01.2009 21:22:
>>>> On 1/11/09 4:05 PM, Ingo Struewing wrote:
>>> Yes. Keeping the write direction open means that the client can still
>>> receive data. The worst thing that can happen is that the client gets an
>>> error on an attempt to write to the socket. If it interprets this as
>>> "Lost connection to server", then we've won anyway. Just an error
>>> message from the server might get lost.
>> My doubt is about a client polling his socket and bailing out in case
>> poll returns a error if one of the directions is closed. This needs to
>> be documented as a protocol change.
Scratch this, I was wondering about something else.
> Ok. Documenting the half-close in KILL situations is surely a good
> thing. However, i don't know where to look for the protocol
> documentation. Can you help out?
> OTOH we would need a "broken" implementation of the Berkeley sockets. A
> reasonably conforming implementation would not take notice of the close
> of the read direction on the peer. Unless it tries to actually write to
> the socket. A poll on the write direction will only notice the
> transition from a full write buffer to a non-full write buffer.
> Anyway, if a poll misinterprets the half-close as connection failure of
> some kind, it could try to work around it, or close by itself. In the
> former case the full close follows soon. Finally the closed state is
> reached in any case. The only problem might be wrong error messages.
Have you tested? Out of curiosity, I looked at Linux's kernel IPv4
shutdown function (tcp_shutdown) and it actually does nothing if the
receiving side (SHUT_RD) is to be closed. I also went on to test on OS X
and writes will succeed even if the receive operations on the server are
disabled. SHUT_RD only seems to be meaningful on pipes and alike. Is
this really worth the effort?
>>> I would like to keep adding sync points as easy as possible. If
>>> debug_sync.h is included in mysql_priv.h, it is available in most files.
>>> No extra check for "#include<debug_sync.h>" needs to be done after
>>> adding a sync point. Since debug_sync.h has protection against double
>>> use, I'll leave the explicit uses in the files where you added them.
>> Please, don't add it back to mysql_priv.h.. easy of use is not a concern
> This might be a misunderstanding. I do not want to add back the
> definitions. But just an #include<debug_sync.h>. As I proposed in my patch.
The end result is the same.
> If this is, what you thought, but still object, please explain, what the
> problem is.
First of all, adding debug_sync.h to mysql_priv.h as to ease the
addition of sync point is not a acceptable argument as including
debug_sync.h when necessary is a no-brainier. Also, headers should be
self sufficient and minimal and adding more stuff to mysql_priv.h goes
against our modularization efforts, slows down compile time, needlessly
tempers with compiler caches and goes against common coding standards.
I object and I'm pretty sure the second reviewer will object too.
>>>>> + /* purecov: begin inspected */
>>>>> + batch_readline_end(status.line_buff);
>>>>> + /* purecov: end */
>>>> Unrelated, please commit separated so it can be pushed early.
>>> Reporting a bug for this, and waiting for it to become tagged by triage,
>>> may finally take longer than to have it included here.
>> OK to push separately. I didn't suggest a separate bug report.
> Ok, will do. But please note that in former time it was a rule not to
> make patches that are neither related to a bug nor a worklog.
This sends shivers down my spine..