Current implementation of StreamReader does not take care of the status of the buffer, once an exception has been set via `set_exception` any call to the read methods won't be able to get the missing data still pending to be processed.

From the point of view of the developer, if there is no scheduled task for waiting data into the Streamreader between a network data gets into the buffer socket and a closing connection by the other peer arrives, the developer won't be able to gather the previous data.

Do you have a specific use case where it's important to access the remaining data? ISTM that this is happening when the connection is lost and then it shouldn't matter how much of the data you read or not -- the connection was broken before the remote side closed it, and you should not trust the data.

Yeps,

I have an example related to Redis [1], the server warns that the
connection will be closed because you reached out the maximum number
of connections.

But IMHO the example it's just that an example, if the connection at
some point was not broken/closed/... and some data came into the
buffer, this data should be put available for the user.

[1] https://github.com/antirez/redis/blob/09dd7b5ff02de4a311032939c27fd6fc62fbd4a3/src/networking.c#L615

On Wed, Jul 5, 2017 at 11:19 PM, Guido van Rossum
<report@bugs.python.org> wrote:
>
> Guido van Rossum added the comment:
>
> Do you have a specific use case where it's important to access the remaining data? ISTM that this is happening when the connection is lost and then it shouldn't matter how much of the data you read or not -- the connection was broken before the remote side closed it, and you should not trust the data.
>
> ----------
> nosy: +gvanrossum
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue30861>
> _______________________________________

> But IMHO the example it's just that an example, if the connection at
some point was not broken/closed/... and some data came into the
buffer, this data should be put available for the user.

In my experience, you never want to recover any data from broken connections.  In cases where it's safe, StreamReader attaches the buffered data to the exception (see IncompleteReadError).

One of the disadvantages with the Exception is that you are relying on
how the events are being succeeded, and this is something that you
can't predict. With just a different delay between the data and the
RST packet and you might get the data in two different ways, either
via a regular read or via one adhoc Exception.

This might be confusing for the developer and will end up with the
second thing that IMHO is also important, the code that has to be
produced by the developer gets complicated.

On Wed, Jul 5, 2017 at 11:29 PM, Yury Selivanov <report@bugs.python.org> wrote:
>
> Yury Selivanov added the comment:
>
>> But IMHO the example it's just that an example, if the connection at
> some point was not broken/closed/... and some data came into the
> buffer, this data should be put available for the user.
>
> In my experience, you never want to recover any data from broken connections.  In cases where it's safe, StreamReader attaches the buffered data to the exception (see IncompleteReadError).
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue30861>
> _______________________________________

The Redis example doesn't sound valid to me, as (IIUC) it closes the connection immediately?

My 2c:

Pau's concern seems valid, in a sense that stream should work like TCP.
That's what most users would assume -- read out data until the end, only then you can see what the actual error was (socket closed, or timeout or hard error)

However, I suspect the devil may be in details -- the patch appears to assume that exception is only set by the underlying source of data (in which case user wants to read out everything up to the exception).

I can imagine a couple of use-case where an exception is set "out of band":
* to terminate bad reader (e.g. against Slowris attack)
* there's a matching StreamWriter (e.g. shared socket)

P.S.
If I'm wrong here, then bug report and patch must be clearer :)

As was said, the assumption here is the data that came to the buffer must be available.

For example, the next snippet shows a Redis client that expects the data message plus the RST packet, where the redis-server was configured to accept max N connections, the connection used by this snippet was the N+1

import socket
import time

s = socket.socket(
    socket.AF_INET, socket.SOCK_STREAM)
s.connect(("localhost", 6379))

# give enought time to get the
# data plus the RST Package
time.sleep(1)

# read the data
print(s.recv(100))

It works like a charm.

IMHO this is not a matter of RST vs close graceful, its a matter of giving always the chance to the developer to read the data that is still remaining and ready into the buffer.

The current implementation is in somehow not predictable. Different network latencies with the same scenario can produce different outputs and the same with how the read of the Streamer is handled by the developer.

We seem to have a failure to communicate. I'm sure your example code "works", but you're not showing what's in the data it receives that is important for the app to read (your example just prints it).

And surely your app should be robust even if the connection is closed *without* ever receiving that data (since it may be lost in transit).

If you need an application-level error your design should include it in the protocol, not use the connection closing.

I think that we are not in the same page, the following snippet is a nodejs client that emulates the same:

var net = require('net');
var sleep = require('sleep');

var client = new net.Socket();
client.connect(6379, '127.0.0.1', function() {
    console.log('Connected');
    sleep.sleep(10); // wait for data and RST package
});

client.on('data', function(data) {
    console.log('Received: ' + data);
});

client.on('close', function() {
    console.log('Connection closed');
});

The client will be able to get the data, even taking into account that the RST package has been processed by the Operating System. Meanwhile, the same behavior with asyncio is unpredictable.

It seems Guido sets a higher bar on the proposed change.

@pfreixes, if you can show that this change is needed to implement "TCP half-closed" state (i.e. when remote calls shutdown(SHUT_WR) after it's done sending data but continues to recv(), then local is expected to read out the data, and then confirm reception and issue it's own shutdown(SHUT_WR) so that remote gets the acknowledgement), then there would be no question.

But half closed state is already supported.

On Jul 7, 2017 2:37 AM, "Dima Tisnek" <report@bugs.python.org> wrote:

>
> Dima Tisnek added the comment:
>
> It seems Guido sets a higher bar on the proposed change.
>
> @pfreixes, if you can show that this change is needed to implement "TCP
> half-closed" state (i.e. when remote calls shutdown(SHUT_WR) after it's
> done sending data but continues to recv(), then local is expected to read
> out the data, and then confirm reception and issue it's own
> shutdown(SHUT_WR) so that remote gets the acknowledgement), then there
> would be no question.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue30861>
> _______________________________________
>

I would like to focus the issue as was initially described, Im still
convinced that this is a buggy behaviour. As has been seen other systems
such as python sync or nodejs behaves as is expected.

This last one is IMHO something that can be skipped.

Im wondering how curio or trio will handle this scenario. My bet is on as
nodejs.

El 07/07/2017 17:06, "Guido van Rossum" <report@bugs.python.org> escribió:

>
> Guido van Rossum added the comment:
>
> But half closed state is already supported.
>
> On Jul 7, 2017 2:37 AM, "Dima Tisnek" <report@bugs.python.org> wrote:
>
> >
> > Dima Tisnek added the comment:
> >
> > It seems Guido sets a higher bar on the proposed change.
> >
> > @pfreixes, if you can show that this change is needed to implement "TCP
> > half-closed" state (i.e. when remote calls shutdown(SHUT_WR) after it's
> > done sending data but continues to recv(), then local is expected to read
> > out the data, and then confirm reception and issue it's own
> > shutdown(SHUT_WR) so that remote gets the acknowledgement), then there
> > would be no question.
> >
> > ----------
> >
> > _______________________________________
> > Python tracker <report@bugs.python.org>
> > <http://bugs.python.org/issue30861>
> > _______________________________________
> >
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue30861>
> _______________________________________
>

IMO you haven't demonstrated a need, you are just complaining that you
don't like it.

I looked at the PR and it looks good. The intent is also clear. But the idea of delaying exceptions in StreamReader read methods still feels finicky.

To go forward with this we need more examples than just nodejs. And it doesn't really matter what trio and curio do, as they aren't mainstream. We need a (set of) *concrete* examples where this is needed.

An analysis of how this is approached in another mainstream libraries/ecosystems like C#, Twisted, etc would also help.

fair enough. I'm out with few chances to grab a  computer. I'll be back in
a week with more information.

El 07/07/2017 20:14, "Yury Selivanov" <report@bugs.python.org> escribió:

>
> Yury Selivanov added the comment:
>
> I looked at the PR and it looks good. The intent is also clear. But the
> idea of delaying exceptions in StreamReader read methods still feels
> finicky.
>
> To go forward with this we need more examples than just nodejs. And it
> doesn't really matter what trio and curio do, as they aren't mainstream. We
> need a (set of) *concrete* examples where this is needed.
>
> An analysis of how this is approached in another mainstream
> libraries/ecosystems like C#, Twisted, etc would also help.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue30861>
> _______________________________________
>

The following links point to three different implementations of the same scenario using a Twisted, Node and Python blocking clients that try to reproduce the same scenario.

- Twisted https://gist.github.com/pfreixes/0d8b24b98567e557d6059b3308aa07ca
- Node https://gist.github.com/pfreixes/b62c199e62ae09d1a5b9e13652c7a273
- Python blocking https://gist.github.com/pfreixes/7a85e43642782eb7e7d0669eadd0216a

All of them are able to give you the buffer when the RST package was already processed by the OS.

I'm still trying to get a Java client, but TBH I won't expect a different behavior.

There's no need. You seem to have accidentally shown a use case for getting the buffered data -- it can contain an error message explaining why the connection was closed.

The Java client https://gist.github.com/pfreixes/13fedf2a589c260e6c7c64ae73653bb1

Works as is expected the buffer can be read till it gets empty, no matter when the RST was sent.

Yeps, any update on this bug and the fix proposed?