Re: tds_socketpair on windows

On Mon, 2 Apr 2012 19:05:39 +0100
Frediano Ziglio <[email protected]> wrote:

Hi Freddy, 

Thank you for taking the time to explain this.  It's a hard
problem, I agree.  IIUC, one socket in pair is for the tcp/ip
connection to the server, and the other is for signalling (via poll
(2)) that SQLCancel was called.  Whichever comes ready first wakes up
poll(2).  A clever and elegant design.  

Like everything in programming, there are other ways.  And, because I
didn't think of your way, I spent some time thinking of others.  I'd
like to understand the race condition better, because that's crucial.
If there's no race condition -- and I don't see one -- then just
setting a flag suffices.  

You raise some concerns that make the requirements seem stricter than
they really are.  Let me address them.  

> From dblib/ctlib specification cancel can be called from a signal
> handler.

Not according to anything I've read, e.g.:

http://msdn.microsoft.com/en-us/library/aa936950(SQL.80).aspx

http://manuals.sybase.com/onlinebooks/group-cnarc/cng1110e/dblib/@Generic__BookTextView/8979;pt=8800

http://infocenter.sybase.com/help/index.jsp?topic=/com.sybase.help.sdk_12.5.1.ctref/html/ctref/X86092.htm

> handle user input like ctrl-c sending a cancel we must be signal safe.

No.  The application traps the signal and uses the library
appropriately.  It cannot call the library from within the handler.  

> You could try to stop signal, do the checks and then restart signaling

No need.  

> From MS specifications (SQLCancel) cancel can be called from another
> thread.

Yes.  http://msdn.microsoft.com/en-us/library/windows/desktop/ms714112
(v=vs.85).aspx  and
http://msdn.microsoft.com/en-us/library/windows/desktop/ms715361
(v=vs.85).aspx.

So, we could have:

thread #1: pending on any ODBC function
thread #2: SQLCancel on the same statement handle

That means every ODBC function must have a way to notice that SQLCancel
has been called while blocked against I/O.  That is why we use
non-blocking I/O, tds_select(), and poll(2).  

> How cancel should detect that something is working on the socket and
> not break data?

Exactly how we were doing it.  Call poll(2) with a 1-second timeout.
On each timeout look for a cancellation.  If cancelled, do the right
thing.  

> On write is more complicated cause it cannot write and so it has to
> signal the other to send a cancel when it can. But how to signal?
> Setting a variable at first sight seemed ok but it cause a race
> condition that is hard to manage.

This is the crux of the matter.  The simplest thing for SQLCancel to do
is set a flag.  I think it's sufficient, and easier to manage than a
second socket.  

Why is it hard to manage?  SQLCancel sets
is_cancelled = 1. tds_goodread/tds_goodwrite examines is_cancelled on
each 1-second timeout.  It's either true or false.  No race condition
and no problem.  

It's true that a cancel can be messy.  There are two kinds of
cancellation, good and bad:

Good.  The server is notified within the TDS protocol, and acknowledges
the cancel packet.  In this case the connection remains OK and the
cancelled function simply fails.  Processing can continue.  

Bad.  The server is not notified.  The library must close the connection
and set TDS_DEAD.  The function fails and processing cannot continue.  

Reading is normally a Good case: it's possible to send
TDS_BUFSTAT_ATTN and receive TDS_BUFSTAT_ATTNACK.  That can happen on a
timeout or SQLCancel.  It can also happen because of dbcancel() or
SQLFreeStmt() before reading the last row of a result set.  

But what if TDS_BUFSTAT_ATTN cannot be sent, or
TDS_BUFSTAT_ATTNACK is not received?  Then it's a Bad case.  

Writing, as you say, is normally a Bad case.  The client can
always cease sending, but it can't *send* anything while it's blocked.
There's no way to send TDS_BUFSTAT_ATTN.  All it can do (whether
timeout or SQLCancel) is close the connection and mark it DEAD.  

> Just to complicate things to support tls you cannot write to socket
> without checking for read cause on long write to server could require
> crypt key change so we need always to test for read. 

Well, yes, it does complicate things.  Normally reading and
writing are separate: *either* tds_goodread *or* tds_goodwrite is
pending, but not both, and not one calling the other.  But that's just
a little bit of state that has to be managed in the SSL logic.  

In sum:

1.  I'm still not convinced a second socket is needed.  It's clever,
and it appeals to me because I like IPC.  But I don't see the race
condition and I think a simple flag would do the job.  

2.  I'm not saying we need to change anything.  I just want to be clear
about what the needs are, and not to make things more complicated than
they really are.  If a flag really is enough, then there's certainly no
need to introduce proprietary OS calls to replace the "simple" socket.  

Regards, 

--jkl