Check whether it's ok for the Python and Java driver to ignore the response to STOP requests

[danielmewes]

The Python and Java drivers currently ignore the server's response to a STOP request on a cursor.

I'm not sure how the JS and Ruby drivers handle this.

Ignoring the response could hide some real issues.
If for example a bug in the driver or server caused the wrong token to be sent, the server would respond with an error that the token is unknown. However the client would ignore the error. As a consequence, cursors might never get closed, and will fill up memory on the server.

[bchavez]

Continuing the conversation from 9151114#commitcomment-18451665

Do you have some example codes that show how to handle STOP correctly? I'd like to update the C# driver if we really should be checking the STOP response.

I'm guessing it works something like:

Cursor.Close()
 -> conn.Stop()
    -> SendQuery(), but wait for reply
 -> Check Response.IsError *in* Cursor.Close() and throw *in* Cursor.Close() if needed?

🍂 🍃 _"I get a sense of weightlessness…"_

[danielmewes]

@bchavez Yeah, basically just check if the response is an error response, and if it is, throw that error (or log it somewhere, or whatever makes sense).

[AtnNn]

In my experience, unknown token errors should often be ignored. If the server sends a SUCCESS_SEQUENCE and forgets the token while simultaneously the client sends a STOP, the response from the server will be an error about "token not in stream cache".

[danielmewes]

@AtnNn Ok that makes sense. I didn't think of that case.
I guess then logging the response might be useful for debugging, but not otherwise.

[bchavez]

So, I tried to implement this in the C# driver and here's what happened. There are two actions I thought were most reasonable to handle:

Act 1 - Two Threads - One Reader, One Closer

Thread A: Cursor.MoveNextAsync() // and waiting for a response.
Thread B: Cursor.Close() // a cursor close by a different thread.

Act 2 - One Thread - Aborted Read, then Close

Thread A: Cursor.MoveNextAsync() // Thread A aborts the async wait operation.
Thread A: Cursor.Close() // Then comes around and moves to close the cursor.

Solution 1

My first thought for Act 1 was any call to Close() forcibly cancels any pending async operation in MoveNextAsyc. Any future calls to MoveNextAsync just get rejected outright. The thread that called Close wins and gets to read the last response. But, it turns out every Cursor needs to carry around some extra synchronization primitives to pull this off. It's probably okay, but seems like it makes cursors heavy... probably my OCD killing me here... Another disadvantage is the cursor code gets kind of complicated. All these additional semaphore registrations within the OS to handle cross-thread async abort signaling make me weary. Ultimately, two threads need to communicate with each other one way or another about who's going to handle the last read... drama right? yea... hm.

I did it, but it's not pretty... so, bit more thinking on it....

Solution 2

I thought of another solution that used an interlocking mechanism that doesn't need OS semaphore primitives for cross-thread communication. I like it since it keeps cursor objects lightweight. Ye, simple object locking is pretty much handled by the CLR runtime using a thinlock bit flag in a CLR object header (or a quick lookup in the CLR syncblock table). coo. not bad...

But one burning question remains... in Solution 2 is how am I going to know if I'm in Act 1 or Act 2. It's possible I'm in Act 1, but Thread A goes off and does something and maybe never comes back to processes the last STOP message. Hmm. Yea.

One way I'm thinking to discriminate between Act 1 and Act 2 is by asking the user when calling Close() use a parameter Close(bool waitForReplyOnThisThread = false). But it could encourage misusage. If the user lies to me, could potentially result in two PrimeCursor reads of the last response (not that it's a big deal). But yea...

Solution 2.5

Use some kind of atomic interlocked counter to deflect any MoveNextAsync that happen the instant Close happens. Haven't really thought this one through yet.

---

I just dunno. Perhaps I just need to get past the OCD, bite the bullet, and use a SemaphoreSlim for signaling. It's not that bad... If anyone got ideas on how to pull this off in a more lightweight (or simpler) way, hook me up. 🔌 💡

Come to think of it... starting to wonder, I haven't had a bug in the cursor code since February'16 when I rewrote it... my .NET peeps seem happy. If it ain't broke, why fix it?

💨 🚶 _"Bubbles of gas in my brain... Send me off balance, it's not enough"_

[danielmewes]

@bchavez I'm now tending towards ignoring the STOP result. It becomes rather complicated whenever there are concurrent outstanding requests on the cursor.

In some simpler drivers (like the PHP driver), handling the STOP response is easy. That's just because it doesn't allow multi-threaded access and doesn't perform any pre-fetching of cursor results though.

[NotJustAnna]

The Java driver currently have a PR (rethinkdb/rethinkdb-java#12) that rewrote the way Cursors work (by removing them and replacing them with reactive streams), and still had no use to the response of the STOP. I assume this issue can be safely closed. You can discuss it in a new issue under the new repository (https://github.com/rethinkdb/rethinkdb-java) or on the PR itself.