New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #6693 - FastCGI review #6694
Conversation
- Removed code supporting multiplex in the client. - Removed code supporting multiplex in the server. - Reworked the server-side processing of a request, now more similar to HTTP/1.1. - Improved javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Fixed javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few minor cleanups that could be done, but otherwise LGTM.
jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java
Outdated
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
Outdated
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
Outdated
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
Outdated
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
Show resolved
Hide resolved
jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/ServerFCGIConnection.java
Show resolved
Hide resolved
Updates after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
private final HttpFields.Mutable fields = HttpFields.build(); | ||
private final Dispatcher dispatcher; | ||
private HttpInput.Content _specialContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other fields here are a bit of a mix of _ and no _. Can you correct them all to be consistent.
if (!hasContent()) | ||
return null; | ||
|
||
HttpInput.Content content = _contentQueue.poll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need a queue here? onContent now always returns true to ensure back pressure, so we should not get more than one content.
Also should we rename the action in the parse from ASYNC to IN_CONTENT?
Updates after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet simone.bordet@gmail.com