unbounded memory in streamable transport

[jba]

In the current implementation of the streamable HTTP transport, the server can consume unbounded memory.

1. stream.outgoing is a slice of messages ready to send. But they may be sent very slowly if the client connection is slow. What's worse, if the client never does a non-replay GET (and the spec says that it doesn't have to), then messages for stream 0 will never be sent.

2. Since a stream can always be replayed, even after the response to a request is sent, data structures for the stream are kept forever.

For (1) and POST requests, we should bound the outgoing queue to exert backpressure on the server.

For (1) and the non-replay GET, bounding the queue will just lead to deadlock if the GET never arrives. However, dropping messages that are sent prior to receiving the GET is also wrong: it will cause a race condition where the server might send a message on a session just as it's established and before the client can make the GET request. One solution is to use a circular queue and drop old messages. These messages should be notifications for the most part (feature-changed, logging) so dropping them doesn't matter. If the server were to make requests outside of a client request, dropping those would be more problematic.

[jba]

/cc @findleyr @samthanawalla

[samthanawalla]

I can work on this.

To break it down:

• For POST request streams, replace stream.outgoing with bounded queue (buffered channel)
• Replace the notification stream (stream 0) with a circular buffer
• Enforce a configurable TTL eviction policy to purge inactive streams

[jba]

If the server were to make requests outside of a client request, dropping those would be more problematic.

We should also talk about this case. For now, I would error out if a request is sent to stream 0. It would be bad to drop requests.

I believe this is a flaw in the spec. It should either change the MAY to a MUST for the GET (and require that it happen ASAP, though even that wouldn't truly avoid the possibility of server memory overflow if the server sent millions of requests immediately); or the spec should say that server requests that aren't part of of a client request MAY fail (or better, MUST fail). If you have a better idea, let's discuss. Otherwise I'll file an issue against the spec.

[jba]

Enforce a configurable TTL eviction policy to purge inactive streams

Let's keep this as simple as possible: a single duration.

[samthanawalla]

To implement the eviction policy for the MemoryEventStore, I was thinking of doing a LRU cache style system.

We store a "last accessed" parameter which refreshes during an Append or After call and keeps a time sorted list of last accessed streams (using the doubly linked list stdlib package for efficiency). Then we have a janitor goroutine which periodically checks the back of the list to evict it.

We could also use an existing cache library but I figured it might be overkill.

What do you think?