Skip to content

Conversation

@shishirb-MSFT
Copy link
Collaborator

No description provided.

@shishirb-MSFT shishirb-MSFT requested a review from a team as a code owner August 31, 2021 20:26
@shishirb-MSFT shishirb-MSFT force-pushed the user/shishirb/ni-server-impl-rebase branch from 009e96c to c6ec8af Compare August 31, 2021 20:50
RETURN_HTTP_STATUSCODE_STR(BadRequest);
RETURN_HTTP_STATUSCODE_STR(NotFound);
RETURN_HTTP_STATUSCODE_STR(InternalError);
RETURN_HTTP_STATUSCODE_STR(ServiceUnavailable);
Copy link
Contributor

@jimson-msft jimson-msft Aug 31, 2021

Choose a reason for hiding this comment

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

If it's not one of these 5, then the http status code string is set to "StatusDescription" in HttplistenerConnection::Reply(), is this intended? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we currently don't use anything else. The description actually doesn't matter in the protocol since it is purely for humans to read.

ss << "Content-Length: " << body.size() << "\r\n";
}
ss << "Server: Delivery-Optimization-Agent/" << microsoft::deliveryoptimization::util::details::SimpleVersion() << "\r\n";
ss << "\r\n";
Copy link
Contributor

@jimson-msft jimson-msft Aug 31, 2021

Choose a reason for hiding this comment

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

This crlf is used for the empty header correct? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it signifies the end of the header section.

if (ec)
{
DoLogError("Async accept failed, error = %d", ec.value());
// TODO(shishirb) terminate process by throwing exception?
Copy link
Contributor

@jimson-msft jimson-msft Aug 31, 2021

Choose a reason for hiding this comment

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

Would it make sense to try again up until n failures? #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. The only time I've seen a failure here is during shutdown notifying us that socket is closed (expected). So I'll leave it as is until we see other retry-able errors.

Copy link
Contributor

@jimson-msft jimson-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@shishirb-MSFT shishirb-MSFT merged commit 98e6dfa into feature/replace-cpprest Aug 31, 2021
@shishirb-MSFT shishirb-MSFT deleted the user/shishirb/ni-server-impl-rebase branch August 31, 2021 22:56
shishirb-MSFT added a commit that referenced this pull request Aug 31, 2021
* Remove cpprest from listener
* Listener conns keep alive. Track num connections.
* Send agent version in REST response.
* SDK: Fix rest_interface_tests
shishirb-MSFT added a commit that referenced this pull request Sep 16, 2021
* Remove cpprest from listener
* Listener conns keep alive. Track num connections.
* Send agent version in REST response.
* SDK: Fix rest_interface_tests
shishirb-MSFT added a commit that referenced this pull request Sep 16, 2021
* Remove cpprest from listener
* Listener conns keep alive. Track num connections.
* Send agent version in REST response.
* SDK: Fix rest_interface_tests
shishirb-MSFT added a commit that referenced this pull request Sep 16, 2021
* Remove cpprest from listener
* Listener conns keep alive. Track num connections.
* Send agent version in REST response.
* SDK: Fix rest_interface_tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants