Add a variant of configureAsyncHTTPServerPipeline which takes a stream delegate

[adam-fowler]

In the async HTTP2 pipeline there is no way to account for the opening and closing of streams. This PR adds a variant of configureAsyncHTTPServerPipeline (HTTP2 upgrade channel) that allows the user to pass in a NIOHTTP2StreamDelegate to be applied to the HTTP2 channel if the upgrade occurs.

This is similar to #439 which did the same HTTP2 only channels.

[Lukasa]

Hi @adam-fowler, can you write a quick test that actually uses this function? Doesn't need to be complex, just something to confirm that it works and to ensure we won't regress it.

[adam-fowler]

whoops forgot to do that

[adam-fowler]

done

[adam-fowler]

I've found I can't configure the HTTP1 channel either now. Would it be easier to make configureHTTP2AsyncSecureUpgrade public. Then I can provide the http1 and http2 channel setups I would prefer, without adding a mountain of new functions.

[Lukasa]

I'd like @FranzBusch to weigh in here if that's ok.

[FranzBusch]

I've found I can't configure the HTTP1 channel either now. Would it be easier to make configureHTTP2AsyncSecureUpgrade public. Then I can provide the http1 and http2 channel setups I would prefer, without adding a mountain of new functions.

In general, I am not opposed to making that method public but it is also trivial to reimplement outside of this module. I would like to understand what you mean with "can't configure the HTTP1 channel" since there is an initialiser for incoming HTTP/1 connections in your API here.

[adam-fowler]

I've found I can't configure the HTTP1 channel either now. Would it be easier to make configureHTTP2AsyncSecureUpgrade public. Then I can provide the http1 and http2 channel setups I would prefer, without adding a mountain of new functions.

In general, I am not opposed to making that method public but it is also trivial to reimplement outside of this module. I would like to understand what you mean with "can't configure the HTTP1 channel" since there is an initialiser for incoming HTTP/1 connections in your API here.

Actually you are right there, I can implement configureHTTP2AsyncSecureUpgrade outside of NIOHTTP2. I thought it used internal symbols but it seems it doesn't. Although I think it would be nicer for users of the library if they didn't have to.

Regarding the function I am adding here if the user wanted to configure the HTTP1 channel, it would require adding a bunch more parameters to this function ie all the parameters that configureHTTPServerPipeline currently takes. What do you think? Seems like quite a few extra parameters, I'm sure this could structured better.

[adam-fowler]

So I got around this by duplicating configureHTTP2AsyncSecureUpgrade in the HB codebase so don't actually need this function anymore. Do you still want it? Or should I close this

[Lukasa]

I'm inclined to still want to take something in this space. @glbrntt was the last to work in this area of the codebase and so probably has the most informed opinion.

[adam-fowler]

Well from my point of view making configureHTTP2AsyncSecureUpgrade public is the most useful thing that could be done. Then individuals can do the following

configureHTTP2AsyncSecureUpgrade { channel in
    setupHTTP1Channel()
} http2ConnectionInitializer: { channel in
    setupHTTP2Channel()
}

Which isn't far from the EventLoop API

channel.configureHTTP2SecureUpgrade(
    h2ChannelConfigurator: { channel in
        setupHTTP2Channel()
    },
    http1ChannelConfigurator: { channel in
        setupHTTP1Channel()
    }
)

[glbrntt]

I think this makes sense to add, but can you de-dupe some of the code with the other helper?

[glbrntt]

Looks great, thanks a lot Adam!

[glbrntt]

There are a couple of formatting bits that need fixing up:

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift:768:21: warning: [OmitExplicitReturns] 'return' can be omitted because body consists of a single expression
Sources/NIOHTTP2/HTTP2PipelineHelpers.swift:768:21: warning: [OmitExplicitReturns] 'return' can be omitted because body consists of a single expression
Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:610:25: warning: [OmitExplicitReturns] 'return' can be omitted because body consists of a single expression
Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:1:62: warning: [OmitExplicitReturns] 'return' can be omitted because body consists of a single expression
Tests/NIOHTTP2Tests/ConfiguringPipelineAsyncMultiplexerTests.swift:610:25: warning: [OmitExplicitReturns] 'return' can be omitted because body consists of a single expression

[adam-fowler]

@glbrntt seems like some of the actions failed because of timeouts when setting up