Skip to content

Add an implementation of reactor http server #4946

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

sky92zwq
Copy link

While developing an HTTP framework based on POCO for my client, I realized POCO didn't provide an implementation of a reactor HTTP server. So I decided to implement a reactor HTTP server. There aren't too many differences between HTTP servers and HTTP reactor servers from the user's perspective. I just added several TCP parameters and changed the HTTP server to an HTTP reactor server, which is shown in the example HTTPReactorTime.

I later found some similar work [here](#2093). I may have duplicated some effort.

If anyone is interested in this, please let me know.

Copy link
Member

@aleks-f aleks-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you. I tried to do it long time ago but it was never finished/merged.

I did not yet have time for a thorough review, but in principle I would welcome this contribution.

While at it, you may also want to replace the NotificationCenter with AsyncNotificationCenter - it's a drop-in replacement that should work out of the box. See #4414 and #4851 for the reason.

Oh yes, please write some unit tests. And see CodingStyle.

namespace Net {



Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coding style:

  • remove indentation for the second namespace
  • remove blank line between namespaces
  • there should be only two blank lines after second namespace

};

}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing EOF newline

}
}

#endif // Net_HTTPReactorServerSession_INCLUDED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing EOF newline

}


#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing EOF newline

} // namespace Net
} // namespace Poco

#endif // Net_TCPReactorServer_INCLUDED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing EOF newline

#include <cstring>

namespace Poco {
namespace Net {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no indentation

_tcpReactorServer.stop();
}

void HTTPReactorServer::onMessage(const TcpReactorConnectionPtr & conn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: opening brace in new line

response.setVersion(request.getVersion());
response.setKeepAlive( request.getKeepAlive() && session.canKeepAlive());
try
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style:

  • space/tab indentation mix
  • too much indentation

void setReactorMode(bool reactorMode);
/// Sets the reactor mode.
///
/// If true, use reactor mode, else use thread pool mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: there should be a blank line between comment and the next function

@aleks-f aleks-f requested review from obiltschnig and matejk May 15, 2025 15:34
@sky92zwq
Copy link
Author

Interesting, thank you. I tried to do it long time ago but it was never finished/merged.

I did not yet have time for a thorough review, but in principle I would welcome this contribution.

While at it, you may also want to replace the NotificationCenter with AsyncNotificationCenter - it's a drop-in replacement that should work out of the box. See #4414 and #4851 for the reason.

Oh yes, please write some unit tests. And see CodingStyle.

OK!

@sky92zwq sky92zwq force-pushed the feat/reactor-http-server branch from 0a60c52 to aeb05c0 Compare May 16, 2025 13:59
@matejk
Copy link
Contributor

matejk commented Jun 9, 2025

@sky92zwq , many of the tests fail. Please check the logs and correct the code.

@sky92zwq
Copy link
Author

@matejk I have added some commits, but I don't have a Windows compilation environment locally. May this pull request (PR) automatically trigger the workflow?

@aleks-f
Copy link
Member

aleks-f commented Jun 10, 2025

@matejk I have added some commits, but I don't have a Windows compilation environment locally. May this pull request (PR) automatically trigger the workflow?

@sky92zwq I have approved the CI run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants