Removed soft cancellation for the reader's sequential methods#3276
Conversation
There was a problem hiding this comment.
Thanks @vonzshik.
I was thinking about these cases in the last few days (WaitAsync, COPY, sequential read), and how we can deal with them. Here's a summary of scenarios:
| Scenario | Attempt PG cancellation? | Break on timeout? |
|---|---|---|
| Normal command | Yes | Yes |
| Sequential read inside message | No | Yes |
| Reading from any type handler | No | Yes |
| COPY export | Yes (only for CopyData) | Yes |
| COPY import | No | Yes |
| WaitAsync | No | No |
Some notes on this table:
- Type handler reads are basically sequential reads - since type handlers read inside a column, everything is buffered except when we're doing sequential.
- Basically, whenever we're accessing the buffer directly (not via NpgsqlConnector.ReadMessage), we never want PG cancellation (this is the case of sequential and type handler reads).
- When we're reading messages (via NpgsqlConnector.ReadMessage), we want PG cancellation, except for Wait.
- For COPY export, things align nicely with the above: when reading CopyData we want cancellation, when reading within CopyData we don't.
Re implementation... for break vs. don't break on timeout, we pass a readingNotifications flag to NpgsqlReadBuffer.Ensure - it makes sense to me to do the same for whether PG cancellation should be attempted or not (e.g. attemptPostgresCancellation. This PR basically does the same thing, except that it passes that flag through AsyncLocal, which I think is complicates our flow/logic (which is already complicated), and spreads out state in another place.
As per the above, the default overload of Ensure - the one called by everyone except NpgsqlConnector.ReadMessage - should not attempt PG cancellation. So ReadMessage can call a special overload which does attempt it.
roji
left a comment
There was a problem hiding this comment.
Looks great, see some comments below.
roji
left a comment
There was a problem hiding this comment.
Looks almost ready to merge, there's still some questions of when to do PG cancellation, see comments below.
|
|
||
| CopyOutResponseMessage copyOutResponse; | ||
| var msg = _connector.ReadMessage(); | ||
| var msg = _connector.ReadMessageWithoutCancellation(); |
There was a problem hiding this comment.
Didn't we say we want PG cancellation when reading messages when exporting?
Thinking about this again, we can say that we don't think PG cancellation makes sense with COPY, because we don't think PG will hang for any reason (after all it's just pushing out data), so we only hang because of network issues. If this is true (it makes sense to me), we can just do hard cancellation. Interestingly we still allow sending PG cancellation via the Cancel method, but that wouldn't be because of hanging, just because the user decided mid-way to stop the long process.
I'm still not completely sure here.
There was a problem hiding this comment.
But we're not reading here a CopyDataMessage. Is there a reason you think, that reading a CopyOutResponseMessage may hang?
There was a problem hiding this comment.
Also, just like RawCopyStream , here we're initializing the stream. Is it really OK to cancel, when we do not even know if the copy operation has really started?
There was a problem hiding this comment.
To be honest, I have no idea...
Let's go with your approach, and attempt cancellation only when reading CopyData. We can always adjust this later based on user feedback if we get any.
| { | ||
| Expect<ParseCompleteMessage>(connector.ReadMessage(), connector); | ||
| var paramTypeOIDs = Expect<ParameterDescriptionMessage>(connector.ReadMessage(), connector).TypeOIDs; | ||
| Expect<ParseCompleteMessage>(connector.ReadMessageWithoutCancellation(), connector); |
There was a problem hiding this comment.
Any reason this shouldn't do PG cancellation?
There was a problem hiding this comment.
This one is... actually, I'm not really sure about. Does it really can timeout because it was executing for too long?
There was a problem hiding this comment.
Probably not - we're just asking the server to parse and describe the parameters. I think in my mind I wanted to default to trying cancellation, but like with COPY we can go with this and revisit later (this API specifically is in very low usage.
| var task = reader.GetFieldValueAsync<byte[]>(0, cts.Token); | ||
| cts.Cancel(); | ||
|
|
||
| var exception = Assert.ThrowsAsync<OperationCanceledException>(async () => await task); |
There was a problem hiding this comment.
For completeness we can assert that no PG cancellation request reached the server (also in the other relevant tests).
roji
left a comment
There was a problem hiding this comment.
OK, you've pretty much convinced me :)
Am going to merge as-is, we'll revisit in the future based on feedback.
Closes #3275