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

natsConnection_RequestMsg returns NATS_NO_SERVER_SUPPORT instead of NATS_TIMEOUT when there is no connection #644

Closed
fran6co opened this issue Mar 8, 2023 · 11 comments · Fixed by #645

Comments

@fran6co
Copy link

fran6co commented Mar 8, 2023

This happens only when sending messages with headers, if no headers are sent it behaves normally.

        natsMsg *msg = nullptr;
        auto status = natsMsg_Create(&msg, subject.c_str(), nullptr, (const char*) message->data(), (int)message->size());
        if (NATS_OK != status){
            throw std::runtime_error("Issue creating requesting msg" + std::string(nats_GetLastError(&status)));
        }

        // If this line is removed we get NATS_TIMEOUT instead of NATS_NO_SERVER_SUPPORT
        if (natsMsgHeader_Set(msg, "some_header", "some_value") != NATS_OK) {
            throw std::runtime_error("Couldn't set the header");
        }

        natsMsg *replyMessage = nullptr;
        status = natsConnection_RequestMsg(&replyMessage, connection_handle, msg, timeout);
        natsMsg_Destroy(msg);
        if(NATS_TIMEOUT == status || NATS_NO_RESPONDERS == status){
            return nullptr;
        }

        // We shouldn't get here if there is no server running
        if(NATS_OK != status){
            throw std::runtime_error("Issue requesting: " + std::string(nats_GetLastError(&status)));
        }
@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

I can't reproduce this. Are you sure you are not connected to a server that does not support headers? Headers support require a server v2.2.0. Could you check?

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

And what do you mean when there is no connection? The server is stopped or the connection is closed before calling requestMsg? The latter should result in connection closed error.

@fran6co
Copy link
Author

fran6co commented Mar 8, 2023

I mean no server running at all at any point. If you just run the client without any broker running and have a header it will fail with the other error

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

I am fairly convinced that you are using an older server (v2.1.x or older). But I will close this issue only after you confirm it.

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

Sorry, our messages crossed. Let me try that.

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

I mean no server running at all at any point. If you just run the client without any broker running and have a header it will fail with the other error

But you can't do that. The connection would not be able to be created, so you would not have a connection handle to call natsConnection_RequestMsg().
I am really confused...

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

Again, if you have a valid connection, and suppose the server is stopped (but the connection is still valid - not closed yet, trying to reconnect), the code that you show in your report will work as expected if you were connected to a v2.2.0+ server. I have checked and it works for me. If you are running a v2.1.x-, then yes, you would get "no supported" because of the use of headers, which the library knows that a server v2.2.0+ is required for that.

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

In other words, when the library tries to send a message that has headers, it will return the error if the last "INFO" protocol from the server does not indicate that headers are supported. See

if (!nc->info.headers)

And it is not possible to publish if you did not have a valid connection at one point, meaning it has to have been connected once.

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

And in the very narrow case where you would be using the natsOptions_SetRetryOnFailedConnect() option and indeed the code you showed execute before the library had a chance to connect, then yes, you would get the NATS_NO_SERVER_SUPPORT error because the library has no knowledge if the server (or lack thereof) supports headers. So you should probably add check for this error status too.

@fran6co
Copy link
Author

fran6co commented Mar 8, 2023

Sorry I didn't create a full example, we are using natsOptions_SetRetryOnFailedConnect so we can restart the broker or start the client apps before the broker. I haven't done the timing but it does look like it might be respecting the timeout while without headers it seems to wait for the timeout before failing even without a connection

@kozlovic
Copy link
Member

kozlovic commented Mar 8, 2023

@fran6co Correct. The current behavior would cause the natsConnection_RequestMsg() to fail right away if not yet connected and if the message has headers. I can fix that so that the header's server support check is done only when actually connected (or at least a first connection was established). I will submit a PR today.

kozlovic added a commit that referenced this issue Mar 9, 2023
…R_SUPPORT

When using `natsOptions_SetRetryOnFailedConnect` option to connect
and the library is not yet connected, calling `natsConnection_RequestMsg`
with a message with headers would incorrectly return `NATS_NO_SERVER_SUPPORT`
instead of `NATS_TIMEOUT`.

Resolves #644

Signed-off-by: Ivan Kozlovic <[email protected]>
kozlovic added a commit that referenced this issue Mar 9, 2023
…R_SUPPORT

When using `natsOptions_SetRetryOnFailedConnect` option to connect
and the library is not yet connected, calling `natsConnection_RequestMsg`
with a message with headers would incorrectly return `NATS_NO_SERVER_SUPPORT`
instead of `NATS_TIMEOUT`.

Resolves #644

Signed-off-by: Ivan Kozlovic <[email protected]>
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 a pull request may close this issue.

2 participants