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

Add ssl:getstat/1 and ssl:getstat/2 #987

Closed
wants to merge 2 commits into from
Closed

Add ssl:getstat/1 and ssl:getstat/2 #987

wants to merge 2 commits into from

Conversation

essen
Copy link
Contributor

@essen essen commented Mar 9, 2016

These functions call getstat on the underlying TCP socket.
The only way to do this before now was to use a hack, either
by looking inside the #sslsocket{} record directly, or by
not using the SSL listen/accept functions and upgrading
from a TCP socket that is kept around for the purpose of
calling getstat later on.

A separate commit also exports the type inet:stat_option/0
to be able to use it in the ssl module's specs.

Rebase of #929 against master, along with requested test fixes.

@OTP-Maintainer
Copy link

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@IngelaAndin
Copy link
Contributor

Have not had time to analyse this yet but currently tests are failing. This should be the most relevant part of the log:

*** CT Error Notification 2016-03-22 04:30:59.955 ***
ssl_basic_SUITE:getstat failed on line 603
Reason: {badmatch,[false,true]}

Full error description and stacktrace

*** User 2016-03-22 04:30:59.955 ***
ssl_test_lib:97
Server Msg: ok

=== Ended at 2016-03-22 04:30:59
=== Location: [{ssl_basic_SUITE,getstat,603},
{test_server,ts_tc,1533},
{test_server,run_test_case_eval1,1053},
{test_server,run_test_case_eval,985}]
=== Reason: no match of right hand side value [false,true]
in function ssl_basic_SUITE:getstat/1 (ssl_basic_SUITE.erl, line 603)
in call from test_server:ts_tc/3 (test_server.erl, line 1533)
in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1053)
in call from test_server:run_test_case_eval/9 (test_server.erl, line 985)

For use in ssl:getstat/{1,2}.
These functions call getstat on the underlying TCP socket.
The only way to do this before now was to use a hack, either
by looking inside the #sslsocket{} record directly, or by
not using the SSL listen/accept functions and upgrading
from a TCP socket that is kept around for the purpose of
calling getstat later on.
@essen
Copy link
Contributor Author

essen commented Apr 1, 2016

@IngelaAndin I can't reproduce.

Considering the code, what happens is probably a timing issue, so I introduced two timer:sleep(100) after sending and before checking that the send stats were updated. I hope that's OK.

@OTP-Maintainer
Copy link

Patch has passed first testings and has been assigned to be reviewed


I am a script, I am not human


@IngelaAndin
Copy link
Contributor

This PR has been stalled in waiting for my rewrite of ssl to use gen_statem. I included in a first version into our builds yesterday, and it will be merged for 19-RC1. I know that it will take care of some timing issues and hopefully these issues too.

@psyeugenic psyeugenic added team:PS Assigned to OTP team PS feature labels Apr 18, 2016
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Apr 26, 2016
@essen
Copy link
Contributor Author

essen commented May 13, 2016

This wasn't merged for 19-RC1 (or wasn't in the README). Is this still going in for 19?

@IngelaAndin
Copy link
Contributor

This is a small addition in comparison to the gen_statem rewrite, and does not have a big impact on other parts, so it may till make it for 19. Several ssl PR are stalled as gen_statem rewrite has been prioritized. I just put this one into the test machinery again. Alas the case is failing on 22 of 28 machines

ssl_basic_SUITE:getstat failed on line 651
Reason: {badmatch,[false,true]}

I understand that this is due to timing issues in the test case. We need to try and find a way around this. I will try to help but to be frank I have some other things higher on the prio list at the moment.

@essen
Copy link
Contributor Author

essen commented May 13, 2016

Fair enough, statem is more important, just tell me when you got time.

@IngelaAndin IngelaAndin added testing currently being tested, tag is used by OTP internal CI and removed stalled waiting for input by the Erlang/OTP team labels Jun 1, 2016
@IngelaAndin
Copy link
Contributor

I changed you timer:sleep to _ = ssl:connection_information(Socket), which makes things alot better.
Alas a few platforms will still get the result:

** CT Error Notification 2016-06-03 05:05:00.438 ***
ssl_basic_SUITE:getstat failed on line 734
Reason: {badmatch,[false,true]}


 732:     wait_for_send(PassiveC),
  733:     {ok, SStats} = ssl:getstat(PassiveC, [send_cnt, send_oct, send_avg]),
  734:     [true] = lists:usort([proplists:get_value(Name, SStats) =/= proplists:get_value(Name, InitialStats)
  735:         || Name <- [send_cnt, send_oct, send_avg]]),

@essen
Copy link
Contributor Author

essen commented Jun 3, 2016

Which platforms?

@IngelaAndin
Copy link
Contributor

Linux, windows7 and openbsd 5.9. I am not sure if is true for all of the failing machines but at least one of them is slow machine.

@IngelaAndin
Copy link
Contributor

It is 5 failing out of 33 so far (still 10 machines running and 2 aborted) . Before my change it failed on 20+ machines.

@IngelaAndin
Copy link
Contributor

You can not depend on send_avg having changed. I changed the test to just use send_cnt and sned_oct hopefully it will pass now.

@essen
Copy link
Contributor Author

essen commented Jun 8, 2016

Ah right, it's an average... feels silly

@IngelaAndin
Copy link
Contributor

Merged with my updates to the tests.

@IngelaAndin IngelaAndin closed this Jun 9, 2016
@IngelaAndin IngelaAndin removed the testing currently being tested, tag is used by OTP internal CI label Jun 9, 2016
@essen
Copy link
Contributor Author

essen commented Jun 9, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants