Pull Request: Refactor FastCGI Client into Separate Package (#4378)

modules/caddyhttp/reverseproxy/fastcgi/client_test.go

@@ -38,6 +38,9 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/fastcgi/fcgiclient"
+	"go.uber.org/zap"
 )
 
 // test fcgi protocol includes:
@@ -123,7 +126,7 @@ func sendFcgi(reqType int, fcgiParams map[string]string, data []byte, posts map[
 		return
 	}
 
-	fcgi := client{rwc: conn, reqID: 1}
+	fcgi := fcgiclient.NewClient(conn, zap.NewNop(), true)
 
 	length := 0
 

modules/caddyhttp/reverseproxy/fastcgi/fastcgi.go

@@ -30,11 +30,10 @@ import (
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy"
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp/reverseproxy/fastcgi/fcgiclient"
 	"github.com/caddyserver/caddy/v2/modules/caddytls"
 )
 
-var noopLogger = zap.NewNop()
-
 func init() {
 	caddy.RegisterModule(Transport{})
 }
@@ -167,12 +166,11 @@ func (t Transport) RoundTrip(r *http.Request) (*http.Response, error) {
 	}()
 
 	// create the client that will facilitate the protocol
-	client := client{
-		rwc:    conn,
-		reqID:  1,
-		logger: logger,
-		stderr: t.CaptureStderr,
-	}
+	client := fcgiclient.NewClient(
+		conn,
+		logger,
+		t.CaptureStderr,
+	)
 
 	// read/write timeouts
 	if err = client.SetReadTimeout(time.Duration(t.ReadTimeout)); err != nil {

modules/caddyhttp/reverseproxy/fastcgi/client.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/client.go

@@ -21,7 +21,7 @@
 // Use of this source code is governed by a BSD-style
 // Part of source code is from Go fcgi package
 
-package fastcgi
+package fcgiclient
 
 import (
 	"bufio"
@@ -125,13 +125,38 @@ var pad [maxPad]byte
 // client implements a FastCGI client, which is a standard for
 // interfacing external applications with Web servers.
 type client struct {
-	rwc net.Conn
+	// This is the underlying network connection that the client uses to communicate with the FastCGI server. It could be a TCP socket or any other type of connection supported by standard Go's net package.
+	// The conn field is essential for sending FastCGI requests and receiving responses from the server.
+	conn net.Conn
+
 	// keepAlive bool // TODO: implement
-	reqID  uint16
+
+	// In the FastCGI protocol, each request is assigned a unique identifier (ID). This field stores the ID of the current request being handled by the client.
+	// Request IDs help differentiate multiple concurrent requests and ensure proper response routing.
+	// TODO: in current Caddy implementation of FastCGI this is always set to 1
+	// This field would have different values when implemented keepAlive (see above)
+	requestID uint16
+
+	// This boolean flag indicates whether the client should capture and handle standard error (stderr) output from the FastCGI application.
+	// When set to true, the client can redirect stderr output for logging, debugging, or error reporting.
 	stderr bool
+
+	// This is a pointer to a logger instance from the zap logging library. zap is a popular high-performance logging framework in Go.
+	// The logger field allows the client to log events, errors, and debugging information during its operation.
 	logger *zap.Logger
 }
 
+func NewClient(conn net.Conn, logger *zap.Logger, stderr bool) client {
+	client := client{
+		conn:      conn,
+		requestID: 1,
+		stderr:    stderr,
+		logger:    logger,
+	}
+
+	return client
+}
+
 // Do made the request and returns a io.Reader that translates the data read
 // from fcgi responder out of fcgi packet before returning it.
 func (c *client) Do(p map[string]string, req io.Reader) (r io.Reader, err error) {
@@ -231,7 +256,7 @@ func (c *client) Request(p map[string]string, req io.Reader) (resp *http.Respons
 
 	// wrap the response body in our closer
 	closer := clientCloser{
-		rwc:    c.rwc,
+		rwc:    c.conn, // maybe change this clientCloser field's name from rwc to conn?
 		r:      r.(*streamReader),
 		Reader: rb,
 		status: resp.StatusCode,
@@ -348,7 +373,7 @@ func (c *client) PostFile(p map[string]string, data url.Values, file map[string]
 // fcgi responder. A zero value for t means no timeout will be set.
 func (c *client) SetReadTimeout(t time.Duration) error {
 	if t != 0 {
-		return c.rwc.SetReadDeadline(time.Now().Add(t))
+		return c.conn.SetReadDeadline(time.Now().Add(t))
 	}
 	return nil
 }
@@ -357,7 +382,7 @@ func (c *client) SetReadTimeout(t time.Duration) error {
 // the fcgi responder. A zero value for t means no timeout will be set.
 func (c *client) SetWriteTimeout(t time.Duration) error {
 	if t != 0 {
-		return c.rwc.SetWriteDeadline(time.Now().Add(t))
+		return c.conn.SetWriteDeadline(time.Now().Add(t))
 	}
 	return nil
 }

modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/fastcgi.go

@@ -0,0 +1,19 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package fcgiclient
+
+import "go.uber.org/zap"
+
+var noopLogger = zap.NewNop()

modules/caddyhttp/reverseproxy/fastcgi/header.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/header.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package fastcgi
+package fcgiclient
 
 type header struct {
 	Version       uint8

modules/caddyhttp/reverseproxy/fastcgi/pool.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/pool.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package fastcgi
+package fcgiclient
 
 import (
 	"bytes"

modules/caddyhttp/reverseproxy/fastcgi/reader.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/reader.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package fastcgi
+package fcgiclient
 
 import (
 	"bytes"
@@ -27,7 +27,7 @@ type streamReader struct {
 
 func (w *streamReader) Read(p []byte) (n int, err error) {
 	for !w.rec.hasMore() {
-		err = w.rec.fill(w.c.rwc)
+		err = w.rec.fill(w.c.conn)
 		if err != nil {
 			return 0, err
 		}

modules/caddyhttp/reverseproxy/fastcgi/record.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/record.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package fastcgi
+package fcgiclient
 
 import (
 	"encoding/binary"

modules/caddyhttp/reverseproxy/fastcgi/writer.go → modules/caddyhttp/reverseproxy/fastcgi/fcgiclient/writer.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package fastcgi
+package fcgiclient
 
 import (
 	"bytes"
@@ -29,12 +29,12 @@ type streamWriter struct {
 }
 
 func (w *streamWriter) writeRecord(recType uint8, content []byte) (err error) {
-	w.h.init(recType, w.c.reqID, len(content))
+	w.h.init(recType, w.c.requestID, len(content))
 	w.buf.Write(pad[:8])
 	w.writeHeader()
 	w.buf.Write(content)
 	w.buf.Write(pad[:w.h.PaddingLength])
-	_, err = w.buf.WriteTo(w.c.rwc)
+	_, err = w.buf.WriteTo(w.c.conn)
 	return err
 }
 
@@ -129,10 +129,10 @@ func (w *streamWriter) writeHeader() {
 
 // Flush write buffer data to the underlying connection, it assumes header data is the first 8 bytes of buf
 func (w *streamWriter) Flush() error {
-	w.h.init(w.recType, w.c.reqID, w.buf.Len()-8)
+	w.h.init(w.recType, w.c.requestID, w.buf.Len()-8)
 	w.writeHeader()
 	w.buf.Write(pad[:w.h.PaddingLength])
-	_, err := w.buf.WriteTo(w.c.rwc)
+	_, err := w.buf.WriteTo(w.c.conn)
 	return err
 }