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.

Sign up for GitHub

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

[web] fix rest-test issues #593

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

[web] fix rest-test issues #593

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a43127a
[web] fix rest-test issues
tttttangTH Oct 19, 2020
7a4769c
resolve comment
tttttangTH Oct 20, 2020
50baa34
Merge branch 'main' of https://github.com/openthread/ot-br-posix into…
bukepo May 13, 2021
0e7a66a
Merge branch 'main' of https://github.com/openthread/ot-br-posix into…
bukepo May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 7 additions & 21 deletions src/rest/resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,6 @@ Resource::Resource(ControllerOpenThread *aNcp)
mResourceCallbackMap.emplace(OT_REST_RESOURCE_PATH_DIAGNOETIC, &Resource::HandleDiagnosticCallback);
}

void Resource::Init(void)
{
otThreadSetReceiveDiagnosticGetCallback(mInstance, &Resource::DiagnosticResponseHandler, this);
}

void Resource::Handle(Request &aRequest, Response &aResponse) const
{
std::string url = aRequest.GetUrl();
Expand Down Expand Up @@ -518,31 +513,22 @@ void Resource::UpdateDiag(std::string aKey, std::vector<otNetworkDiagTlv> &aDiag

void Resource::Diagnostic(const Request &aRequest, Response &aResponse) const
{
otbrError error = OTBR_ERROR_NONE;
otThreadSetReceiveDiagnosticGetCallback(mInstance, &Resource::DiagnosticResponseHandler,
Copy link
Member

Choose a reason for hiding this comment

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

why setting callback for each Diagnostic is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, If factoryreset is called, the callback may be bind to another function. We should ensure that each time diagnostic API in resource is called, the response message can be collected by our resource handler.

Copy link
Member

Choose a reason for hiding this comment

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

After factoryreset, Resource::Init will run again and sets up the callback ? Is the original code causing real issues?

Copy link
Contributor Author

@tttttangTH tttttangTH Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Resource::Init will only be run once when we start otbr-agent, then factoryreset will set the callback to the default(seems a handler for CLI), I am not sure whether there is another approach that set otThreadSetReceiveDiagnosticGetCallback to another function. do you think we do this each time before diagnostics API is called is acceptable? or is there an approach that we could detect whether factoryreset is called?

Copy link
Member

Choose a reason for hiding this comment

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

Resource::Init will only be run once when we start otbr-agent, then factoryreset will set the callback to the default(seems a handler for CLI), I am not sure whether there is another approach that set otThreadSetReceiveDiagnosticGetCallback to another function. do you think we do this each time before diagnostics API is called is acceptable? or is there an approach that we could detect whether factoryreset is called?

I think factoryreset re-exec the whole process, so the process restarts as if it was just launched normally. We should not blame factoryreset for any issue. If multiple components of ot-br-posix is using otThreadSetReceiveDiagnosticGetCallback, maybe they should coordinate with each other.
Should we use EventEmitter so that ot-br-posix calls otThreadSetReceiveDiagnosticGetCallback just once and let other components subscribe to the corresponding event using EventEmitter::On? @gjc13 @tttttangTH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will try to do like this.

const_cast<Resource *>(this));
OT_UNUSED_VARIABLE(aRequest);
struct otIp6Address rloc16address = *otThreadGetRloc(mInstance);
struct otIp6Address multicastAddress;

VerifyOrExit(otThreadSendDiagnosticGet(mInstance, &rloc16address, kAllTlvTypes, sizeof(kAllTlvTypes)) ==
OT_ERROR_NONE,
error = OTBR_ERROR_REST);
VerifyOrExit(otIp6AddressFromString(kMulticastAddrAllRouters, &multicastAddress) == OT_ERROR_NONE,
error = OTBR_ERROR_REST);
OT_ERROR_NONE);
VerifyOrExit(otIp6AddressFromString(kMulticastAddrAllRouters, &multicastAddress) == OT_ERROR_NONE);
VerifyOrExit(otThreadSendDiagnosticGet(mInstance, &multicastAddress, kAllTlvTypes, sizeof(kAllTlvTypes)) ==
OT_ERROR_NONE,
error = OTBR_ERROR_REST);
OT_ERROR_NONE);

exit:

if (error == OTBR_ERROR_NONE)
{
aResponse.SetStartTime(steady_clock::now());
aResponse.SetCallback();
}
else
{
ErrorHandler(aResponse, HttpStatusCode::kStatusInternalServerError);
}
aResponse.SetStartTime(steady_clock::now());
aResponse.SetCallback();
Comment on lines -540 to +532
Copy link
Member

Choose a reason for hiding this comment

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

For now, what happens if we fail in this function? Simply ignore the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that It may fail when node is detached or no buffer for message.

So I think one solution is to define more HTTP status code according to these newly added error code, I think I could do it after #532 is merged.

Another solution is like this, we ignore all errors(more simple, but still reasonable).

For one thing, it could address the no buffer problem - although we fail in this API call, if we have diagnostic information(received in 4s but due to another call) left in resource, this information is still valid and could be sent as response.
For another thing, we could just view empty response as "some issues happen, we can't get anything now".

Copy link
Member

@wgtdkp wgtdkp Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Simply ignore the error?

@tttttangTH Is this true for your changes? I didn't see why we cannot handle the possible failures in this PR. It looks to me that keeping

else
    {
        ErrorHandler(aResponse, HttpStatusCode::kStatusInternalServerError);
    }

should just work. Why did you remove this error handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because if we call otThreadSendDiagnosticGet too many times within a specific period, we will get a no buffer error for this call( for example, when we send 10 requests for diagnostics concurrently , the no buffer error usually occurs).

But actually I prefer not viewing it as an error for HTTP response because the reason for calling otThreadSendDiagnosticGet each time when the server received a request for diagnostics is to update the information we maintained(we have a hash table to record all the diagnostic information we received within 4 seconds). If we have a 'no buffer' error, it means we have call the API many times recently so information in the hash table is almost the latest.

I know the right way to deal with this problem is to make an exception for no buffer rather than remove the error handler. I expect to do this after #532 because I think I may need to rewrite error handler of all RESTful API according to the modifications in #532 . So here I just ignore it currently.

Copy link
Member

@simonlingoogle simonlingoogle Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why is no buffer error not good for such situation? I think no buffer error was exactly what happened and should be reflected by the HTTP response.

Maybe we can return 507 Insufficient Storage for no buffer error.

But if the test is sending too much requests and is expecting no buffer error to happen sometimes, the test client can conclude with success even if there are some no buffer errors.

Thoughts? @tttttangTH @wgtdkp

Copy link
Member

Choose a reason for hiding this comment

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

@tttttangTH You should not just remove the error handler. You can at least add a log for this error.

I know the right way to deal with this problem is to make an exception for no buffer rather than remove the error handler. I expect to do this after #532 because I think I may need to rewrite error handler of all RESTful API according to the modifications in #532 .

I don't see why we need to wait for #532. Are there any REST feature that depends on #532?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have't found any other feature depend on #532 , but it seems we won't catch no buffer at otThreadSetReceiveDiagnosticGetCallback before #532 is applied.

I noticed no buffer before when when I wrote this API, at that time, no buffer was just an Info, and I have chosen to ignore it at that time. But It‘s thoughtless for me to just remove the error handler here.

Copy link
Member

@wgtdkp wgtdkp Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Don't need to address it in this PR: the function name SetCallback is bad... I would expect this function accepts a callback function as an argument but it is actually a flag indicates whether we have/enable callback. Can we rename to SetHasCallback?

Copy link
Contributor Author

@tttttangTH tttttangTH Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, will do it in #537

}

void Resource::DiagnosticResponseHandler(otMessage *aMessage, const otMessageInfo *aMessageInfo, void *aContext)
Expand Down
7 changes: 0 additions & 7 deletions src/rest/resource.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,6 @@ class Resource
*/
Resource(ControllerOpenThread *aNcp);

/**
* This method initialize the Resource handler.
*
*
*/
void Init(void);

/**
* This method is the main entry of resource handler, which find corresponding handler according to request url
* find the resource and set the content of response.
Expand Down
2 changes: 0 additions & 2 deletions src/rest/rest_web_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ otbrError RestWebServer::Init(void)
{
otbrError error = OTBR_ERROR_NONE;

mResource.Init();

error = InitializeListenFd();

return error;
Expand Down
4 changes: 4 additions & 0 deletions tests/rest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ set_tests_properties(rest-server PROPERTIES
set_tests_properties(rest-server PROPERTIES
LABELS "TESTREST"
)

set_tests_properties(rest-server PROPERTIES
TIMEOUT 60
)
6 changes: 6 additions & 0 deletions tests/rest/test-rest-server
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ main()
sudo "${CMAKE_BINARY_DIR}"/src/agent/otbr-agent -d 7 -v -I wpan0 "spinel+hdlc+forkpty://$(command -v ot-rcp)?forkpty-arg=1" &
trap on_exit EXIT
sleep 5
sudo "${CMAKE_BINARY_DIR}"/third_party/openthread/repo/src/posix/ot-ctl factoryreset
sleep 1
sudo "${CMAKE_BINARY_DIR}"/third_party/openthread/repo/src/posix/ot-ctl ifconfig up
sleep 1
sudo "${CMAKE_BINARY_DIR}"/third_party/openthread/repo/src/posix/ot-ctl thread start
sleep 3
sudo python3 "${CMAKE_CURRENT_SOURCE_DIR}"/test_rest.py
}

Expand Down
11 changes: 7 additions & 4 deletions tests/rest/test_rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@


def get_data_from_url(url, result, index):
response = urllib.request.urlopen(urllib.request.Request(url))
body = response.read()
data = json.loads(body)
result[index] = data

try:
response = urllib.request.urlopen(urllib.request.Request(url))
body = response.read()
data = json.loads(body)
result[index] = data
except urllib.error.HTTPError as e:
print(e.code)

def get_error_from_url(url, result, index):
try:
Expand Down