DMTF / Redfish-Protocol-Validator

The Redfish Protocol Validator tests the HTTP protocol behavior of a Redfish service to validate that it conforms to the Redfish specification.
Other
14 stars 12 forks source link

Issue in SERV_SSE_CLOSE_CONNECTION_IF_EVENT_DEST_DELETED #63

Open arunthomasbaby opened 1 year ago

arunthomasbaby commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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"
zuojiachuanren commented 1 month ago

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 commented 1 month ago

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

Thanks, SolonFan

mraineri commented 1 month ago

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.