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

Issue in SERV_SSE_CLOSE_CONNECTION_IF_EVENT_DEST_DELETED #63

Open
arunthomasbaby opened this issue Jul 7, 2023 · 11 comments
Open

Issue in SERV_SSE_CLOSE_CONNECTION_IF_EVENT_DEST_DELETED #63

arunthomasbaby opened this issue Jul 7, 2023 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@arunthomasbaby
Copy link

In test_sse_close_connection_if_event_dest_deleted link to code , we are deleting the event destination URI and checking the SSE stream to confirm whether the stream got closed or not.

The comment says # give the service up to 5 seconds to close the stream link to code but in the logic it will just wait for one second before reading from the sse_stream and break the loop directly and throw the error without waiting for remaining 4 seconds link to code.

Whether this logic is correct ? or we can give a delay of 5 seconds directly before looping the stream link to code

@mraineri
Copy link
Contributor

mraineri commented Jul 7, 2023

Entirely possible something might be wrong here; it's not very obvious to me...

Will look at it further. I know the reason it doesn't wait for a flat 5 seconds is it's trying to shave off a bit of time to move on with other testing.

@arunthomasbaby
Copy link
Author

@mraineri Do we have any further updates on this? Is it OK to replace the sleep(1) with sleep(5) without having the loop?

@mraineri
Copy link
Contributor

No updates yet; I haven't had a chance to debug this further.

I would like to try to maintain the interval method of querying the service; I can see someone asking later that they have a service that takes longer to delete the event destination, and bumping up that sleep value can negatively impact test time for others.

@mraineri mraineri self-assigned this Aug 7, 2023
@mraineri
Copy link
Contributor

mraineri commented Aug 7, 2023

I need to find a system with SSE capabilities to confirm, but it looks like this is as intended; it seems like while in the for loop, "sse_response" will be updated behind the scenes when the connection is closed.

@mraineri
Copy link
Contributor

Sorry this took so long, but here's what I'm observing from the block of code in test_sse_close_connection_if_event_dest_deleted

        for _ in range(5):
            time.sleep(1)
            for _ in sse_response:
                break
            else:
                sut.log(
                    Result.PASS, 'DELETE', r.status_code, event_dest_uri,
                    Assertion.SERV_SSE_CLOSE_CONNECTION_IF_EVENT_DEST_DELETED,
                    'Test passed')
                return

The statement for _ in sse_response will break out and fall into the else case if the SSE stream is closed. So, that part works okay when everything is running as expected.

However, if the previous DELETE operation on the subscription does not ultimately close the stream, that statement will sit until it times out reading data, or consumes some data and on the next loop throws a "StreamConsumedError" exception. This doesn't really seem like a great way to see if the connection is still alive, but so far I don't see anything in the requests module that makes this easy to check either.

@arunthomasbaby
Copy link
Author

arunthomasbaby commented Sep 14, 2023

@mraineri Thanks for the response!!.
We also faced a issue like even if the SSE stream got closed after deleting the subscriptions, already consumed data in the stream is returned before the loop fails.
Lets say, some event occurred before the stream got closed, those events were returned in the statement for _ in sse_response and once all data is read from the stream and since stream is closed, it breaks out. This happens when I increased the outer loop count. Is this expected?

Also we faced some other issue with the script. One is regarding the content type header for Post requests.
In certain post requests, the content type is framed as 'Content-Type': 'application/json;charset=utf-8' . But the OpenBMC BMCWEB, except content type to be 'Content-Type': 'application/json; charset=utf-8' . Link to code
One space is missing between
application/json; and charset . Due to this we get a unknown content type error.
There are other places as well in the script in such a way. Whether this can be updated?

Also in utils.py class SSEClientTimeout(sseclient.SSEClient): whether for data in super(SSEClientTimeout, self)._read(): is blocking until the stream contains data? I am facing a situation were this read is getting blocked until the stream has some data in it. When there is no data in stream, its getting blocked until data is available in stream. Even though we have registered a timeout of 3 seconds, its computed only when _read() returns. In my case, my read() is blocking until I trigger a test event when all the data in the stream is read before 3 seconds .

@mraineri
Copy link
Contributor

Lets say, some event occurred before the stream got closed, those events were returned in the statement for _ in sse_response and once all data is read from the stream and since stream is closed, it breaks out. This happens when I increased the outer loop count. Is this expected?

With the way this loop works today, this seems to be the case. I would really like to try to find alternatives to this check that do not rely on reading out stream data.

Also we faced some other issue with the script. One is regarding the content type header for Post requests. In certain post requests, the content type is framed as 'Content-Type': 'application/json;charset=utf-8' . But the OpenBMC BMCWEB, except content type to be 'Content-Type': 'application/json; charset=utf-8' . Link to code One space is missing between application/json; and charset . Due to this we get a unknown content type error. There are other places as well in the script in such a way. Whether this can be updated?

I'll need to do a bit of research on this; my impression is a whitespace after the semicolon is optional. Please file a separate issue on this, and I'll dig into the HTTP RFCs to see if it really is optional or not.

Also in utils.py class SSEClientTimeout(sseclient.SSEClient): whether for data in super(SSEClientTimeout, self)._read(): is blocking until the stream contains data? I am facing a situation were this read is getting blocked until the stream has some data in it. When there is no data in stream, its getting blocked until data is available in stream. Even though we have registered a timeout of 3 seconds, its computed only when _read() returns. In my case, my read() is blocking until I trigger a test event when all the data in the stream is read before 3 seconds .

I see that too. It looks like that's just how the requests module is designed; read is blocking until data is ready. That's part of the challenge I'm seeing for solving this. I wouldn't mind calling into a read function to get all data if available, but having it sit until a timeout is causing other problems. In some cases it terminates the connection when the timeout is reached, which defeats the purpose of this test.

@mraineri
Copy link
Contributor

I do see this called out in RFC7231 under "Media Type":

     media-type = type "/" subtype *( OWS ";" OWS parameter )
     type       = token
     subtype    = token

OWS is defined as an optional white space. Later in the same section, they state this as an example:

   A parameter value that matches the token production can be
   transmitted either as a token or within a quoted-string.  The quoted
   and unquoted values are equivalent.  For example, the following
   examples are all equivalent, but the first is preferred for
   consistency:

     text/html;charset=utf-8
     text/html;charset=UTF-8
     Text/HTML;Charset="utf-8"
     text/html; charset="utf-8"

@mraineri mraineri added the bug Something isn't working label Jun 26, 2024
@zuojiachuanren
Copy link

zuojiachuanren commented Jul 17, 2024

I am a newcomer to the BMC industry。 I had the same problem recently. This error occurs when the stream does not close in time after deleting an event subscription. But I don't know why.

@solonfan
Copy link

@mraineri Do we have any update about this issue? This issue block our tesing.

Thanks,
SolonFan

@mraineri
Copy link
Contributor

No updates unfortunately. I would recommend ignoring the failure in the report for the time being; if this is not possible, we can disable the test for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants