Skip to content
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

[3.2] Replace websocketpp with boost beast #675

Closed
wants to merge 4 commits into from

Conversation

ClaytonCalabrese
Copy link
Contributor

Overhauls http_plugin, replacing websocketpp with boost::beast. Feature parity with previous implementation, with exception of new additions keep-alive/multiple requests per session and additional server-side error logging. Additional test in plugin_http_api_test for keep-alive.
EOSIO/eos#10583

Resolves #148

praphael and others added 2 commits July 13, 2022 16:20
@ClaytonCalabrese ClaytonCalabrese added 3.2 Candidate OCI OCI working this issue... labels Jul 14, 2022
@ClaytonCalabrese ClaytonCalabrese self-assigned this Jul 14, 2022
@heifner
Copy link
Member

heifner commented Jul 14, 2022

#148 indicated that this could be possibly improved. That very well may be the case, but this is, I believe, a good improvement to the existing http implementation. I would recommend we bring this in and make any enhancements as a separate task @spoonincode.

@spoonincode
Copy link
Member

The code is seemingly over verbose and/or bloated. It doesn't need to be 'enhanced' but instead simplified, but I don't see that ever happening once it's merged in as is, which is why I originally indicated to have a discussion on it before blindly taking it.

@stephenpdeos stephenpdeos added the planning Propose change to milestone in next planning meeting label Jul 19, 2022
@arhag arhag removed the planning Propose change to milestone in next planning meeting label Jul 21, 2022
@spoonincode
Copy link
Member

A couple low effort comments from starting to get in to this again:

  • websocketpp code from fc can be removed along with this PR hopefully
  • I very rarely nit on whitespace (beyond a few of the cornerstones like 3 space tabs and attaching pointer/reference symbols to the type name and not the variable name), but there are a number of cases of brand new files that are inconsistent with their whitespace in even two functions in the same class right next to each other. For example
            // Start accepting incoming connections
            void start_accept()
            {
                if(!is_listening_) return;
                do_accept();
            }

            bool is_listening() {
                return is_listening_;
            }

I'm not sure if maybe we should run this through a clang-format fixup?

@ClaytonCalabrese
Copy link
Contributor Author

I'm not sure if maybe we should run this through a clang-format fixup?

That seems reasonable because I agree the code does seem to not follow a single convention. Do you have a preferred style (LLVM, Google, Chromium, Mozilla, WebKit, Microsoft) you would like me to use?

@heifner
Copy link
Member

heifner commented Aug 1, 2022

That seems reasonable because I agree the code does seem to not follow a single convention. Do you have a preferred style (LLVM, Google, Chromium, Mozilla, WebKit, Microsoft) you would like me to use?

We have talked about this for years. What about just going with a .clang-format, I don't particularly care the exact style. Here is one from CLion that I think closely matches what we use in most files:

# Generated from CLion C/C++ Code Style settings
BasedOnStyle: LLVM
AccessModifierOffset: -3
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: None
AlignOperands: Align
AllowAllArgumentsOnNextLine: false
AllowAllConstructorInitializersOnNextLine: false
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortBlocksOnASingleLine: Always
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: All
AllowShortIfStatementsOnASingleLine: Always
AllowShortLambdasOnASingleLine: All
AllowShortLoopsOnASingleLine: true
AlwaysBreakAfterReturnType: None
AlwaysBreakTemplateDeclarations: Yes
BreakBeforeBraces: Custom
BraceWrapping:
  AfterCaseLabel: false
  AfterClass: false
  AfterControlStatement: Never
  AfterEnum: false
  AfterFunction: false
  AfterNamespace: false
  AfterUnion: false
  BeforeCatch: false
  BeforeElse: false
  IndentBraces: false
  SplitEmptyFunction: false
  SplitEmptyRecord: true
BreakBeforeBinaryOperators: None
BreakBeforeTernaryOperators: true
BreakConstructorInitializers: BeforeColon
BreakInheritanceList: BeforeColon
ColumnLimit: 0
CompactNamespaces: false
ContinuationIndentWidth: 6
IndentCaseLabels: true
IndentPPDirectives: None
IndentWidth: 3
KeepEmptyLinesAtTheStartOfBlocks: true
MaxEmptyLinesToKeep: 2
NamespaceIndentation: None
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PointerAlignment: Left
ReflowComments: false
SpaceAfterCStyleCast: true
SpaceAfterLogicalNot: false
SpaceAfterTemplateKeyword: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: true
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: Never
SpaceBeforeRangeBasedForLoopColon: false
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 0
SpacesInAngles: false
SpacesInCStyleCastParentheses: false
SpacesInContainerLiterals: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
TabWidth: 3
UseTab: Never

@ClaytonCalabrese
Copy link
Contributor Author

Closing due to migration to leap AntelopeIO/leap#22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2 Candidate OCI OCI working this issue...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace websocketpp with boost beast as nodeos' HTTP server implemention
6 participants