add FastCgiHttpMessageHandler with basic tests #2572
Conversation
|
Interesting the whole thing is just a HttpMessageHandler. Does it have uses outside of yarp that would warrant it being its own project? |
|
I tried to make it as HttpMessageHandler to see how far I can go with it. It turn out doable with little tricks like DOCUMENT_ROOT is passed as HttpOption in request - its static option so it could be also passed in constructor. |
|
@Tratcher Tests are still just for me to debug responses (I will add more & better at some point), but code of FastCgiHttpMessageHandler can be reviewed. I need some feedback to known if I'm going in a right direction with it. |
| fastCgiRecord.Dispose(); | ||
| return true; | ||
| } | ||
| case FastCgiRecordHeader.RecordType.EndRequest: |
There was a problem hiding this comment.
Are trailers supported by the protocol?
There was a problem hiding this comment.
Whole HTTP response in FastCgi Response is just "ContentData" in STDOUT record.
There is no special support for Header, Body, or Trailers. FastCgiParams are only for Request. Response is just bytes that need to be parsed. So Trailers are "supported". I need to add tests for it but I think in current implementation I'm sending everything after headers so trailers too, but maybe I need to do something special in HttpResponseMessage?
…mpt of http content data as lazy fasctcgi record stream
| } | ||
| finally | ||
| { | ||
| input.AdvanceTo(buffer.Start, buffer.End); |
There was a problem hiding this comment.
The AdvanceTo can happen first, without a try/finally, because you're not using the content.
| if (prevBuffLenOnCompleted is { } prevLen && prevLen == buffer.Length) | ||
| { | ||
| input.AdvanceTo(buffer.Start, buffer.End); | ||
| throw new BadFastCgiResponseException(FastCgiCoreExpStrings.BadResponse_IncompleteRecord, FastCgiResponseRejectionReason.IncompleteRecord); | ||
| } | ||
|
|
||
| prevBuffLenOnCompleted = buffer.Length; |
There was a problem hiding this comment.
I think it's better to check for this around line 228 and 230. If it can't read the record, and there's still data left, but the pipe is completed, then error.
| if (header.ContentLength == 0) | ||
| { | ||
| fastCgiRecord = new FastCgiRecord( | ||
| Header: header, | ||
| ContentData: ReadOnlySequence<byte>.Empty); | ||
|
|
||
| // Advance the buffer | ||
| buffer = buffer.Slice(recordByteCount); | ||
| return true; | ||
| } | ||
|
|
||
| fastCgiRecord = new FastCgiRecord( | ||
| Header: header, | ||
| ContentData: buffer.Slice(FastCgiRecordHeader.FCGI_HEADER_LEN, header.ContentLength)); | ||
|
|
||
| // Advance the buffer | ||
| buffer = buffer.Slice(recordByteCount); | ||
| return true; |
There was a problem hiding this comment.
Remove duplication
| if (header.ContentLength == 0) | |
| { | |
| fastCgiRecord = new FastCgiRecord( | |
| Header: header, | |
| ContentData: ReadOnlySequence<byte>.Empty); | |
| // Advance the buffer | |
| buffer = buffer.Slice(recordByteCount); | |
| return true; | |
| } | |
| fastCgiRecord = new FastCgiRecord( | |
| Header: header, | |
| ContentData: buffer.Slice(FastCgiRecordHeader.FCGI_HEADER_LEN, header.ContentLength)); | |
| // Advance the buffer | |
| buffer = buffer.Slice(recordByteCount); | |
| return true; | |
| fastCgiRecord = new FastCgiRecord( | |
| Header: header, | |
| ContentData: header.ContentLength == 0 ? | |
| ReadOnlySequence<byte>.Empty : | |
| buffer.Slice(FastCgiRecordHeader.FCGI_HEADER_LEN, header.ContentLength)); | |
| // Advance the buffer | |
| buffer = buffer.Slice(recordByteCount); | |
| return true; |
| _interruptOnState = interruptOnState; | ||
| } | ||
|
|
||
| public ValueTask<FlushResult> FlushAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
Why does a reader have a flush method?
There was a problem hiding this comment.
Because current stream design is like that
First IO Pipe FastCgi Server Connection Reader -> Process FastCGI Record (only 1 STDOUT Record and interrupt) -> Unpack content -> Second IO Pipe Internal Writer Stream -> Flush (make it available for reader) -> Second IO Pipe Reader internal Stream ... so reading stream is causing blocking and waiting for the 1 next frame / record to be buffered in IO Pipe Writer.
Internal Stream Pipe is in FastCgiMessageHandler so that's why it has Flush method.
So its kinda a Extract Transform Load when Reader becomes also feeder/writer to second pipe that is consumed by final client.
| { | ||
| if (!_readerFinished) | ||
| { | ||
| if (await FastCgiRecordReader.ProcessAsync(connection, handler, cancellationToken)) |
There was a problem hiding this comment.
ProcessAsync has a while loop in it that could cause it to read to the end of the response, no?
This Stream.ReadAsync API should return as soon as the given buffer is full. I'd expect a sequence more like
- do I have part of a prior message left? Copy that to the buffer
- while I have more space in the buffer, try reading the next message and copying that to the buffer
- store any partial message for future reads
There was a problem hiding this comment.
No, while loop is always blocking stream( after http Headers) only for just 1 Record/Frame. So it's works similar to what You described, responses (set of records) are partially (frame by frame) buffered in IO PipeWriter and waits for next stream read.
So every 1 Stream read call is causing a buffer filling for only 1 or 0 (finish) Records.
1 frame/record is max 65k bytes - in-practice php-fpm instances are very close to Proxy/Loadbalancer (same machines/node - internal connection) so 1 frame will be transferred very fast and my assumptions is that storing of partial / incomplete frame data is not needed - so I'm waiting until connection IO Reader has full frame.
| public override bool CanSeek => stream.CanSeek; | ||
| public override bool CanWrite => stream.CanWrite; |
| public override bool CanRead => stream.CanRead && !_disposed; | ||
| public override bool CanSeek => stream.CanSeek; | ||
| public override bool CanWrite => stream.CanWrite; | ||
| public override long Length => stream.Length; |
| public override long Length { get; } = 0; | ||
|
|
||
| public override long Position { get; set; } |
| public override void SetLength(long value) => throw new NotSupportedException(); | ||
| } | ||
|
|
||
| private static class HttpResponseHeaders |
There was a problem hiding this comment.
HttpResponseHeaders already exists
| private static class HttpResponseHeaders | |
| private static class FastCgiResponseHeaders |
| void WriteTo(IBufferWriter<byte> buffer); | ||
| } | ||
|
|
||
| private static class FastCgiRecordReader |
There was a problem hiding this comment.
Please split this file up across multiple files by type, reviewing one giant file makes it hard to find things.
|
🙌 |
This allows to send normal HTTP requests using FastCGI protocol.
Implementation is based on Caddy (https://github.com/caddyserver/caddy/tree/master/modules/caddyhttp/reverseproxy/fastcgi). It's a working draft - basic test performed with PHP-FPM & Go.
I need feedback / review - I'm open to discuss how to progress with it and make it mergeable.
Solves: #85