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

Assertion fail when connect and disconnect then cleanup without delay. #60

Open
xpol opened this issue Mar 10, 2016 · 8 comments · May be fixed by #62
Open

Assertion fail when connect and disconnect then cleanup without delay. #60

xpol opened this issue Mar 10, 2016 · 8 comments · May be fixed by #62

Comments

@xpol
Copy link
Contributor

xpol commented Mar 10, 2016

I'm running unit tests on my lua-pomelo got follow error:

Assertion failed: (client->state == PC_ST_CONNECTING || client->state == PC_ST_DISCONNECTING), function pc__trans_fire_event, file /Users/xpol/Workspace/lua-pomelo/deps/libpomelo2/src/pc_trans.c, line 111.

The equal c code is (the c code got same assertion error):

int runc()
{
    pc_client_t* client = (pc_client_t*)malloc(pc_client_size());
    pc_client_config_t config = PC_CLIENT_CONFIG_DEFAULT;
    pc_lib_init(NULL, NULL, NULL, "Test Client");
    pc_client_init(client, NULL, &config);
    pc_client_connect(client, "127.0.0.1", 3010, NULL);
    pc_client_disconnect(client);
    pc_client_cleanup(client);
    return 0;
}

When the follow code executes, the client->state is PC_ST_INITED (1).

function pc__trans_fire_event, file /Users/xpol/Workspace/lua-pomelo/deps/libpomelo2/src/pc_trans.c, line 111.

        case PC_EV_CONNECT_ERROR:
            assert(client->state == PC_ST_CONNECTING || client->state == PC_ST_DISCONNECTING); // <-- client->state is PC_ST_INITED (1)
            break;
@xpol
Copy link
Contributor Author

xpol commented Mar 10, 2016

@cynron
Would you please look into this?
Thanks in advance!

I just can't go on with my unit tests...

@xpol
Copy link
Contributor Author

xpol commented Mar 10, 2016

The problem is:

  1. pc_client_connect() start an async connecting request.
  2. call pc_client_disconnect() immediately, fires PC_EV_DISCONNECT, and pc__trans_fire_event() set client->state to PC_ST_INITED.
  3. tcp__conn_done_cb() called with status == UV_ECANCELED, which fires PC_EV_CONNECT_ERROR.
  4. pc__trans_fire_event() check clinet->state for PC_EV_CONNECT_ERROR, the assertion assert(client->state == PC_ST_CONNECTING || client->state == PC_ST_DISCONNECTING) fails.

The solutions might be:

  1. tcp__conn_done_cb() called with status == UV_ECANCELED don't fire event when client->state == PC_ST_INITED.
  2. tcp__conn_done_cb() called with status == UV_ECANCELED fires another event when client->state == PC_ST_INITED.
  3. or allow fires PC_EV_CONNECT_ERROR in when client->state == PC_ST_INITED.

1 is prefered, 2 is ok, and 3 is just workaround.

What's your option @cynron ?

@xpol
Copy link
Contributor Author

xpol commented Mar 10, 2016

if you choose solution 1, the pr is here #61.

@xpol
Copy link
Contributor Author

xpol commented Mar 10, 2016

I think the function void pc__trans_fire_event(pc_client_t* client, int ev_type, const char* arg1, const char* arg2, int pending) have 2 issues:

  1. should not process sync and async case in one function.
  2. should not assert on the client state because:
    • it maybe called async way, in polling.
    • in release mode assert code is not running.

So, I close #61 , because it does not solve the problem.

@xpol xpol linked a pull request Mar 10, 2016 that will close this issue
@xpol
Copy link
Contributor Author

xpol commented Apr 29, 2016

ping

@lao100
Copy link

lao100 commented Mar 1, 2017

@xpol 现在这个问题解决了吗?

@xpol
Copy link
Contributor Author

xpol commented Mar 1, 2017

@lao100 没,官方对poll模式都没有怎么测试过。

@zengjiwen
Copy link

zengjiwen commented Oct 23, 2022

if (pc_client_state(client) == PC_ST_CONNECTED) {
     pc_client_disconnect(client);
}
pc_client_cleanup(client);

is this work?

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.

3 participants