Cylix / cpp_redis

C++11 Lightweight Redis client: async, thread-safe, no dependency, pipelining, multi-platform - NO LONGER MAINTAINED - Please check https://github.com/cpp-redis/cpp_redis
MIT License
1.24k stars 551 forks source link

Crash when cpp_redis::redis_client is a static variable in Win #166

Closed wenhuancai closed 6 years ago

wenhuancai commented 6 years ago

The problem I encountered was that under win, you had to define the code to not crash.

WORD version = MAKEWORD(2, 2); WSADATA data; if (WSAStartup(version, &data) != 0) { std::cerr << "WSAStartup() failure" << std::endl; return -1; } But if I want to define the global cpp_redis::client client,It has problems. because the global variable definition,WSAStartup is not run.So it is error. I modified the windows_self_pipe. CPP. namespace tacopie { self_pipe::self_pipe(void) : m_fd(__TACOPIE_INVALID_FD) { //! Create a server WORD version = MAKEWORD(2, 2); WSADATA data; WSAStartup(version, &data); m_fd = ::socket(AF_INET, SOCK_DGRAM, 0); if (m_fd == __TACOPIE_INVALID_FD) { __TACOPIE_THROW(error, "fail socket()"); } it is right for running, I feel that the modification is not very good. Is there any other way?

Cylix commented 6 years ago

Hi,

Another way could be to use a wrapper rather than using a static variable.

For example you could have the following:

std::shared_ptr<cpp_redis::client> client_instance;

const std::shared_ptr<cpp_redis::client>& get_redis_instance() {
  if (client_instance == nullptr) {
    WORD version = MAKEWORD(2, 2);
    WSADATA data;
    WSAStartup(version, &data)

    client_instance = std::make_shared<cpp_redis::client>();
  }

  return client_instance;
}

That way, the redis client is still a global instance callable from anywhere, but only created once get_redis_instance() is called the first time, in which case you can do the windows socket initialization only once before you create the instance.

Another way could be to have a wrapper class that contains the redis instance as an attribute and does the windows init in the constructor. This class can be designed as a singleton depending on your need.

class redis_client {
public:
  redis_client() {
    WORD version = MAKEWORD(2, 2);
    WSADATA data;
    WSAStartup(version, &data);

    m_client = std::make_shared<cpp_redis::client>();
  }

private:
  std::shared_ptr<cpp_redis::client> m_client;
}

//! global instance
redis_client client;

Hope this can help and let me know if you need further assistance. Otherwise, let me know if I can close this ticket.

Best

wenhuancai commented 6 years ago

thank you! I'll try it right away.

wenhuancai commented 6 years ago
class GloblaRedis
{
public:
    GloblaRedis();
    ~GloblaRedis();

    cpp_redis::client readRdis;
    cpp_redis::client writeRdis;
public:
    /*static std::shared_ptr<GloblaRedis> get()
    {
        if (nullptr == gClient) {
            WORD version = MAKEWORD(2, 2);
            WSADATA data;
            WSAStartup(version, &data);
             gClient = std::make_shared<GloblaRedis>();;
        }
        return gClient;
    }*/
    public:
        //static GloblaRedis gClient;
   std::shared_ptr<GloblaRedis> gClient;
};

Why you say this method is wrong. image But using my annotated code. Singleton Mode is right. Isn't it wrong to define static variables?

Cylix commented 6 years ago

Defining static variable is not wrong depending on what you do with them. Your example with the commented code is right because that's not the cpp_redis::client which is a static variable, but your GlobalRedis object.

The most important is that you make the order of initialization (between windows init and cpp_redis) defined (thanks to using constructor / global variable / wrapper function / whatever) rather than undefined as before.

I'm closing this ticket as the underlying issue is resolved, but feel free to re-open it if you feel the need.

Best