Correctly discard only inactive streams. #11
Conversation
| private mutating func purgeOldStreams() { | ||
| while self.streamMap.count >= maxSize { | ||
| let lowestStreamID = self.streamMap.filter { $0.value.active }.keys.sorted().first { $0 != 0 && $0 != Int32.max }! | ||
| let lowestStreamID = self.streamMap.filter { !$0.value.active }.keys.sorted().first { $0 != 0 && $0 != Int32.max }! |
There was a problem hiding this comment.
This is the actual fix: I just tackled a TODO while I was here.
There was a problem hiding this comment.
I assume the filtered array (and, by extension, self.streamMap) will now never be empty?
There was a problem hiding this comment.
Not quite, but I wanted to separate the other issue out into another patch.
The TL;DR there is that if the sum of both sides SETTINGS_MAX_CONCURRENT_STREAMS is higher than maxSize, and there are actually more than maxSides valid open active streams, then we'll explode again. That can be fixed by changing the loop conditional above to move the filter out of the loop so that we only care if there are too many closed streams.
This will all get a bit better when we land #10 and the follow-on framing logic such that I can start work on replacing the nghttp2 state machine. At that stage we'll have a much more complete insight into the connection state and won't have to use this somewhat insane purging construction.
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
submitting because page changed
Sources/NIOHTTP2/HTTP2Parser.swift
Outdated
| self.session = NGHTTP2Session(mode: self.mode, | ||
| allocator: ctx.channel.allocator, | ||
| maxCachedStreamIDs: 1024, // TODO(cory): Make configurable | ||
| maxCachedStreamIDs: self.maxCachedClosedStreams, // TODO(cory): Make configurable |
There was a problem hiding this comment.
well, it's now configurable so the comment can go right? Or did you mean ChannelOption kind of configurable?
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
thanks! looks really good! Just nits
| let maxCachedClosedStreams = 1024 | ||
|
|
||
| // Begin by getting the connection up. | ||
| try self.basicHTTP2Connection(maxCachedClosedStreams: 1024) |
There was a problem hiding this comment.
use the maxCachedClosedStreams variable here?
| let maxCachedClosedStreams = 1024 | ||
|
|
||
| // Begin by getting the connection up. | ||
| try self.basicHTTP2Connection(maxCachedClosedStreams: 1024) |
MrMage
left a comment
MrMage
left a comment
There was a problem hiding this comment.
Just a few drive-by comments, sorry ;-)
| let maxCachedClosedStreams = 1024 | ||
|
|
||
| // Begin by getting the connection up. | ||
| try self.basicHTTP2Connection(maxCachedClosedStreams: maxCachedClosedStreams) |
There was a problem hiding this comment.
I was about to mention the following (first part is already fixed): As you are declaring maxCachedClosedStreams above, should you replace 1024 with maxCachedClosedStreams here? Also, would it make sense to pick a lower value than 1024 for faster test runtime?
| try self.assertFramesRoundTrip(frames: [respFrame], sender: self.serverChannel, receiver: self.clientChannel) | ||
| } | ||
|
|
||
| // Ok, now we're going to open *two* streams. In the old, broken code, the opening of the second |
There was a problem hiding this comment.
Mention this PR to more easily reference what the broken code was?
Motivation: Long-lived connections that have many inactive streams would cause crashes. Crashes are, in my professional opinion, less than ideal when they occur under expected usage cases. Modifications: - Fixed an inverted boolean check that would discard only *active* streams. - Made the cached stream count configurable, removing a TODO. - Added tests. Result: Fewer crashes, better software.
|
No need to apologise @MrMage, more code review is always valuable. I've addressed both your comments. |
|
@weissi Want to re-review? |
|
(I'm just a bystander, but) Bump: @weissi this PR fixes an issue discovered in grpc/grpc-swift#281 - just adding a friendly bump for a re-review here :-) |
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
sorry for the delay! thanks @bmhatfield for the reminder
4 participants
Motivation:
Long-lived connections that have many inactive streams would cause
crashes. Crashes are, in my professional opinion, less than ideal
when they occur under expected usage cases.
Modifications:
streams.
Result:
Fewer crashes, better software.