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

mod_offline_SUITE:messages_after_relogin sometimes fails #1799

Closed
arcusfelis opened this issue Apr 12, 2018 · 4 comments
Closed

mod_offline_SUITE:messages_after_relogin sometimes fails #1799

arcusfelis opened this issue Apr 12, 2018 · 4 comments

Comments

@arcusfelis
Copy link
Contributor

MongooseIM version: b076e4a
Installed from: source
Erlang/OTP version: 20

in messages_after_relogin

====== Test name: messages_after_relogin                                                                                          
====== Reason:    {test_case_failed,                                                                                              
                      {has_stanzas_but_shouldnt,                                                                                  
                          {client,<<"alicE@localhost/res1">>,escalus_tcp,                                                         
                              <0.7743.0>,                                                                                         
                              [{event_manager,<0.7740.0>},       
                               {server,<<"localhost">>},                                                                          
                               {username,<<"alicE">>},                                                                            
                               {resource,<<"res1">>}],           
                              [{event_client,                                                                                     
                                   [{event_manager,<0.7740.0>},  
                                    {server,<<"localhost">>},                                                                     
                                    {username,<<"alicE">>},                                                                       
                                    {resource,<<"res1">>}]},     
                               {resource,<<"res1">>},            
                               {username,<<"alicE">>},           
                               {server,<<"localhost">>},         
                               {host,<<"localhost">>},           
                               {port,5222},                      
                               {auth,{escalus_auth,auth_plain}}, 
                               {wspath,undefined},               
                               {username,<<"alicE">>},           
                               {server,<<"localhost">>},         
                               {password,<<"matygrysa">>},       
                               {stream_id,<<"DA4E27366225FAC2">>}]},                                                              
                          [{xmlel,<<"presence">>,                
                               [{<<"from">>,<<"alicE@localhost/res1">>},                                                          
                                {<<"to">>,<<"alice@localhost/res1">>},                                                            
                                {<<"type">>,<<"unavailable">>}], 
                               []}]}} 

detected, during ODBC refactoring.

This version of test would work.

messages_after_relogin(Config) ->                                                    
    %% given                                                                         
    escalus:story(                                                                   
        Config, [{alice, 1}, {bob, 1}],                                              
        fun(User1, User2) ->                                                         
            user_blocks(User1, [User2])                                              
        end),                                                                        
    %% XXX Because alice can receive presence unavalable from alice                  
    %% XXX It's a potential bug, please test it.                                     
    %% XXX has_stanzas_but_shouldnt                                                  
    mongoose_helper:kick_everyone(),                                                 
    escalus:story(                                                                   
        Config, [{alice, 1}, {bob, 1}],                                              
        fun(User1, User2) ->                                                         
            message_is_not_delivered(User2, [User1], <<"Hey alice, are you there?">>),
            message_is_delivered(User1, [User1], <<"Hey bob, carpe diem!">>)         
        end).

But we actually need to fix bug, because this presence unavailable should be not pushed to the client.
I suspect this presence is sent from old alice process to a new one.

kzemek added a commit that referenced this issue Apr 25, 2018
Fixes one common test failure similar to #1799
@arcusfelis
Copy link
Contributor Author

Another similar bug even in 596fbc5

====== Suite FAILED: mod_blocking_SUITE (1 of 20 tests failed)
====== Test name: messages_arrive_after_unblock_and_relogin
====== Reason:    {test_case_failed,
                      {has_stanzas_but_shouldnt,
                          {client,<<"alicE@localhost/res1">>,escalus_tcp,
                              <0.8752.3>,
                              [{event_manager,<0.8751.3>},
                               {server,<<"localhost">>},
                               {username,<<"alicE">>},
                               {resource,<<"res1">>}],
                              [{event_client,
                                   [{event_manager,<0.8751.3>},
                                    {server,<<"localhost">>},
                                    {username,<<"alicE">>},
                                    {resource,<<"res1">>}]},
                               {resource,<<"res1">>},
                               {username,<<"alicE">>},
                               {server,<<"localhost">>},
                               {host,<<"localhost">>},
                               {port,5222},
                               {auth,{escalus_auth,auth_plain}},
                               {wspath,undefined},
                               {username,<<"alicE">>},
                               {server,<<"localhost">>},
                               {password,<<"matygrysa">>},
                               {stream_id,<<"55B52BA9532F2F00">>}]},
                          [{xmlel,<<"presence">>,
                               [{<<"from">>,<<"alicE@localhost/res1">>},
                                {<<"to">>,<<"alice@localhost/res1">>},
                                {<<"type">>,<<"unavailable">>}],
                               []}]}}

@arcusfelis
Copy link
Contributor Author

One way to fix it is in mongoose to force keeping sids (session identifiers) in state of ejabberd_c2s together with presence subscription lists. But it would require more space. In theory, it would allow faster presence update broadcasts (woohoo, without reading session manager). Still. it would require more space to store (i.e. not cool).

Another way is "just" force to use mongoose_helper:kick_everyone() in tests. We can modify escalus story function, to check that resource was released properly. I.e. call mongooseim and wait for the old session process to die. But we would also need a subscription list of the old process (heh).

Another solution just for this set of tests is to use a different resource for each story. If we use "alice@localhost/res1_storyid", than we would avoid the old presences to begin with. Here "storyid" should be a unique value for each call of escalus:story/1. We can implement this "escalus"-wide, but in THEORY it can break some of our tests, that contain resources hardcoded. And some of the tests, that assume, that the resources are always the same.

@michalwski
Copy link
Contributor

@arcusfelis I believe this was fixed already is that right? At least I didn't see it failing for a month already.

@fenek
Copy link
Member

fenek commented Jul 19, 2018

Haven't seen it failing as well. Closing for now.

@fenek fenek closed this as completed Jul 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants