I'll deeply take a look later. At first glance, this approach looks good.

> For Chrome...The problem is that the parent of the new node got finished and 
> removed from the priority tree before the stream id request appeared. I saw 
> this mostly with a high bandwidth client.

This could happen. We have to remove the finished node to keep the tree depth 
shallow ( [256 is current maximum 
depth](https://github.com/apache/trafficserver/blob/4aca609fe08df4b253f8c03aaab6201335c171f2/proxy/http2/Http2DependencyTree.h#L37)
 ). Tracking ancestors looks reasonable. 
Anyway, we should add this scenario to the unit test 
([Http2DependencyTree_Chrome_51](https://github.com/apache/trafficserver/blob/4aca609fe08df4b253f8c03aaab6201335c171f2/proxy/http2/test_Http2DependencyTree.cc#L335)).

> For Firefox, we are inserting nodes in the priority tree for priority frames, 
> but we were treating them as shadow nodes. So once their last direct 
> descendent was deleted, the priority node was also deleted losing the buckets 
> for future requests on the session.

I agree we should leave the node even if the children and queue is empty. The 
`shadow` flag make sense.

> Based on that discussion, I am assuming that shadow nodes are only to 
> represent nodes that will come in the future (e.g. parent id > new stream id).

Practically, this assumption could be true. But theoretically, this could be 
false. Because below case looks allowed. 

- client skip stream 1 ( client can't use this stream in the future )
- client open stream 3 depends on stream 1

[ Full content available at: https://github.com/apache/trafficserver/pull/4212 ]
This message was relayed via gitbox.apache.org for [email protected]

Reply via email to