Conversation
gjcairo
force-pushed
the
max-stream-glitches
branch
2 times, most recently
from
ce2a335 to
a8f05bc
Compare
gjcairo
force-pushed
the
max-stream-glitches
branch
from
6b28a73 to
27a5ef5
Compare
|
The failing allocation test seems to be a regression elsewhere in NIO (see #519) |
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| package struct GlitchesMonitor { |
There was a problem hiding this comment.
Are we sure package visibility is a good idea here? It enables release mode unit tests, sure, but it also causes the optimizer to generate suboptimal code. Given that swift-nio-http2 uses @testable import extensively already, I don't think we gain much by avoiding it here.
There was a problem hiding this comment.
It enables release mode unit tests, sure, but it also causes the optimizer to generate suboptimal code.
Can you expand on why it's suboptimal? Also, should internal + @testable generally be preferred for tests or only in http2 where it's already pervasive?
There was a problem hiding this comment.
It's suboptimal because it inhibits some optimizations that the compiler can make by virtue of being able to observe all usage sites. An internal symbol or type, except when compiled with -enable-testability, has all usage sites visible in the code. This can allow strong effects to be inferred, some types to be deleted, etc.
package visibility doesn't do that: it's essentially the same visibility as -enable-testability would trigger, but all the time. Its saving grace is that it works equivalently well in both modes.
I think package should be preferred in contexts where running the unit tests in release mode is meaningful. I personally don't care much about those, but users who do should go for it. Otherwise, I'd recommend sticking with @testable import.
| public var maximumBufferedControlFrames: Int = 10000 | ||
| public var maximumSequentialContinuationFrames: Int = NIOHTTP2Handler.defaultMaximumSequentialContinuationFrames | ||
| public var maximumRecentlyResetStreams: Int = NIOHTTP2Handler.defaultMaximumRecentlyResetFrames | ||
| public var maximumConnectionGlitches: Int = GlitchesMonitor.defaultMaximumGlitches |
There was a problem hiding this comment.
It would be good, I think, to include narrative docs that explain this field and what it does.
Sources/NIOHTTP2/HTTP2Error.swift
Outdated
| } | ||
| } | ||
|
|
||
| /// The remote peer has triggered too many stream errors on this connection. |
There was a problem hiding this comment.
We might broaden the definition here in future, so let's just call it glitches instead of stream errors.
gjcairo
force-pushed
the
max-stream-glitches
branch
from
3837864 to
db92ada
Compare
3 participants
Add a
GlitchesMonitorthat will terminate a connection if too many stream resets are triggered.