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

Content-Type : text/event-stream always sets the Connection to Close #298

Open
mlozarita opened this issue Jan 27, 2025 · 11 comments
Open
Labels
bug Something isn't working

Comments

@mlozarita
Copy link

Describe the bug
A quick introduction first. I am implementing a REST-API running in STM32 as my target server. I can easily send to the server with POST/GET request and server sending the response with NO problem.

The problem is that when my server responds with a header Content-Type: text/event-stream, the connection got closed and client does not receive any events from the server.

I am using Content-Type: text/event-stream as one of my header parameters because I want to implement Server-Sent Event process (SSE) in my system.

Please also mention any information which could help others to understand
the problem you're facing:

  • What target device are you using?
    STM32H753ZITx.

  • Which version of Eclipse ThreadX?
    ver . 6.4.1

  • What toolchain and environment?
    STMCubeIDE postman as a client.

  • What have you tried to diagnose or workaround this issue?

    I have a work around to this issue to make it work. I have modified 2 files.
    nx_web_http_server.c and nx_tcpserver.c.

In nx_web_http_server.c, function _nx_web_http_server_generate_response_header(),
#ifndef NX_WEB_HTTP_SERVER_OMIT_CONTENT_LENGTH
/* Determine if there is content_length. */
if(content_length || (status_code_not_modified == NX_FALSE)) -->>> Change this from NX_TRUEto NX_FALSE, this is to force the connection to be alive.
{
#ifndef NX_WEB_HTTP_KEEPALIVE_DISABLE

    /* Place the "Connection" field in the header.  */
    if(server_ptr -> nx_web_http_server_keepalive == NX_TRUE)
    {

        /* Keepalive. */
        status += nx_packet_data_append(packet_ptr, "Connection: keep-alive", 22,
                server_ptr -> nx_web_http_server_packet_pool_ptr, NX_WAIT_FOREVER);
    }
    else

#endif /* NX_WEB_HTTP_KEEPALIVE_DISABLE */
{

    /* Close. */
    status += nx_packet_data_append(packet_ptr, "Connection: Close", 17,
                                    server_ptr -> nx_web_http_server_packet_pool_ptr, NX_WAIT_FOREVER);
    }

nx_tcpserver.c., function _nx_tcpserver_timeout_process(), see the last if statement below.

/* Initialize TCP sockets. */
for(i = 0; i < server_ptr -> nx_tcpserver_sessions_count; i++)
{
    session_ptr = &server_ptr -> nx_tcpserver_sessions[i];

    /* Skip socket that is not used. */
    if(session_ptr -> nx_tcp_session_socket.nx_tcp_socket_state == NX_TCP_CLOSED)
    {
        continue;
    }

    /* Skip socket that is already timeout. */
    if(session_ptr -> nx_tcp_session_expiration == 0)
    {
        continue;
    }

   /* Add checking below to skip the timeout when keepalive is enabled. This is to prevent the connection to be disconnected.*/
    **if (session_ptr -> nx_tcp_session_tls_session.nx_secure_tls_tcp_socket->nx_tcp_socket_keepalive_enabled)
    {
    	continue;
    }**

To Reproduce
Steps to reproduce the behavior:

  1. Build my project in STMCubeIDE.

  2. Implement REST-API using NetXduo libraries.

  3. Using postman (as a client), send a POST request to the server with the following http header.
    Content-Type : text/event-stream
    Connection : keep-alive

  4. When the server receives the request command from client that client wants to receive events from the server, then the server will respond with the following http header.
    _nx_web_http_server_generate_response_header.
    Content-Type : text/event-stream
    Connection : keep-alive
    Cache-Control : no-cache

    After sending the above http header to postman, the callback function will return NX_WEB_HTTP_CALLBACK_COMPLETED.

  5. Then in the server, there is a thread that throws a sample event to the postman using the function _nxe_web_http_server_callback_data_send().

Expected behavior
Connection between client and server should always be alive (until it is closed by either the client or server) and when the server sends an event to the client, the client can receive that event.

Impact
We want to use SSE instead of polling mechanism in our project, thus it is kind of a showstopper.

Logs and console output

Additional context
Add any other context about the problem here.

@mlozarita mlozarita added the bug Something isn't working label Jan 27, 2025
@hwmaier
Copy link

hwmaier commented Jan 28, 2025

We did implement SSE and did not have to modify any NetXDuo source code. The problem with keep-alive is that you would need a content length if you use the build in server responses.

You can create your own server response and use that, refer to example below:

    UINT sendSseResponse(NX_WEB_HTTP_SERVER *serverPtr, UINT requestType)
    {
        constexpr char SSE_RESPONSE_HEADER[] =
            NX_WEB_HTTP_VERSION " 200 OK\r\n"
            "Content-Type: text/event-stream\r\n"
            "Cache-Control: no-cache\r\n"
            "Connection: keep-alive\r\n"
            "\r\n";
        UINT rc;
        NX_PACKET *responsePacketPtr;

        if (requestType != NX_WEB_HTTP_SERVER_GET_REQUEST)
            return sendResponse(serverPtr, NX_WEB_HTTP_STATUS_METHOD_NOT_ALLOWED);

        SessionState::getCurrentSession(serverPtr)->setEventSource();

        rc = nx_web_http_server_response_packet_allocate(serverPtr, &responsePacketPtr, NX_WAIT_FOREVER);
        if (rc != NX_SUCCESS)
            return rc;

        rc = nx_packet_data_append(responsePacketPtr, (void *)SSE_RESPONSE_HEADER, strlen(SSE_RESPONSE_HEADER), getPool(), NX_WAIT_FOREVER);
        if (rc != NX_SUCCESS)
        {
            nx_packet_release(responsePacketPtr);
            return rc;
        }

        rc = nx_web_http_server_callback_packet_send(serverPtr, responsePacketPtr);
        if (rc != NX_SUCCESS)
        {
            nx_packet_release(responsePacketPtr);
            return rc;
        }

        // This type of request is kept alive if client has requested so.
        return NX_WEB_HTTP_CALLBACK_COMPLETED; // Success!
    }

@mlozarita
Copy link
Author

Hi Henrik,

Thank you! Appreciate your suggestion here. Yes, I thought I could just create my own server response as well because of that content length but that did not help, and I think that was the reason I have another modification in the nx_tcpserver.c as mentioned above.

Anyway, I followed your code above and tested it, but the connection got closed straight away. See for my test results below.

Image Image

With your code + my modification with nx_tcpserver.c as mentioned above, then it works again.
Image

Am I missing something?

Thank you!

@hwmaier
Copy link

hwmaier commented Jan 28, 2025

If nx_web_http_server_keepalive is set to true server should not disconnect. The SSE client has to open the connection with the keep-alive header set, that's important. Check with Wireshark that your headers from client and server are correctly set and also check who is closing the connection.

@mlozarita
Copy link
Author

Just to add note on this, I can confirm that the nx_web_http_server_keepalive is set to true when function _nx_web_http_server_receive_data() is called.

When I run a debug on the nx_tcpserver.c, I noticed that the nx_tcp_session_expiration got expired in the _nx_tcpserver_timeout_process() which is called in the _nx_tcpserver_thread_entry(), thus closing the connection.
Which leads me to believe that it is the server that is closing the connection. Nonetheless, I can confirm this with wireshark.

@hwmaier
Copy link

hwmaier commented Jan 28, 2025

Ah yes, in our SSE dispatchEvent method which writes the SSE data, we have this line:

            sessionPtr->nx_tcp_session_expiration = serverPtr->nx_web_http_server_tcpserver.nx_tcpserver_timeout;

which prevents the timeout as it resets the time-out counter everytime we write data.

@hwmaier
Copy link

hwmaier commented Jan 28, 2025

I would agree that the SSE support could be better, but instead of changing the time-out code as suggested earlier I rather would add proper methods for sending SSE responses which then set the correct keep-alive headers and reset the timeout counters internally. That then also would not break any existing behaviour.

@mlozarita
Copy link
Author

mlozarita commented Jan 28, 2025

If we let the application handle it, then I think it should be independent from SSE response. What if the next event will happen after some time (ex. in 2 hours), then the connection will be closed again unless if we put that resetting of timeout counters somewhere in a thread that can reset it quickly before it kicks the timeout and disconnect it.

@hwmaier
Copy link

hwmaier commented Jan 28, 2025

There is no harm closing the connection if the events are 2 hours apart. The reason for a keep alive is to minimise connection/disconnection overheads for successive transmissions. It the transmissions are 2 h apart then the overhead of a few milliseconds to re-open the connection does not really matter. The client would reopen it anyway when it wants to send a new event. But if you are streaming events a few milliseconds or per second apart, then you would want to avoid the overhead of open/close and the keep-alive is warranted.

@mlozarita
Copy link
Author

In what I am trying to achieve is that the server will be sending the events to a client. In this case the event will be somewhat real time in the client side instead of client polling data from the server.

Events from the server to the client can be random (not exactly 2 hours interval), therefore that connection should always be in a keep-alive state as much as possible so that server can send events to client anytime.

@fdesbiens
Copy link

Thank you for submitting this issue, @mlozarita. And thank you for jumping in, @hwmaier! We appreciate the constructive feedback you provided.

@eclipse-threadx/iot-threadx-committers and @eclipse-threadx/iot-threadx-contributors: Anything to add to this conversation?

@mlozarita Do you still feel a code change is required?

@mlozarita
Copy link
Author

mlozarita commented Jan 29, 2025

Hi @fdesbiens, I believe it would be great if this can be fix in the library layer rather than in the application layer.

Thank you!

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

3 participants