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

Reworking tcpstreamtest #255

Closed
wants to merge 1 commit into from
Closed

Reworking tcpstreamtest #255

wants to merge 1 commit into from

Conversation

dklimkin
Copy link

@dklimkin dklimkin commented Nov 13, 2017

  1. There was a data race on _running
    In some cases _sock.down() was called before the last read was
    complete. That resulted in fd no longer available exception.
    Also, in theory the thread might never see the value flipped.

  2. Binding to two sockets and reading from the one that's ready on listener thread.
    In some configurations connection gets closed immediately when connection via
    IPv4 address. Used the same approach as in API server (it must be there for a reason).
    Fixes issue tcpsocket accepts on either IPv4 or IPv6 but not both #254.

  3. Generating chars as per constructor parameter.

  4. Connecting to localhost instead of 127.0.0.1, otherwise the test fails on IPv6-only systems.

@dklimkin
Copy link
Author

Not sure similar changes (2) are needed outside of tests, I didn't hit any issues except to the test being flacky.

@dklimkin
Copy link
Author

Can not reproduce test failure locally :(

Using TCP port 4343 for the connection test.
Cycle 0, connection accepted from [::1]:54938
Cycle 1, connection accepted from [::1]:54942
Cycle 1, connection accepted from [127.0.0.1]:44526
Cycle 2, connection accepted from [::1]:54948
Cycle 2, connection accepted from [127.0.0.1]:44530
Cycle 3, connection accepted from [::1]:54952
Cycle 3, connection accepted from [127.0.0.1]:44536
Cycle 4, connection accepted from [::1]:54956
Cycle 4, connection accepted from [127.0.0.1]:44540
Cycle 5, connection accepted from [::1]:54960
Cycle 5, connection accepted from [127.0.0.1]:44544
Cycle 6, connection accepted from [::1]:54964
Cycle 6, connection accepted from [127.0.0.1]:44548
Cycle 7, connection accepted from [::1]:54970
Cycle 7, connection accepted from [127.0.0.1]:44552
Cycle 8, connection accepted from [::1]:54974
Cycle 8, connection accepted from [127.0.0.1]:44558
Cycle 9, connection accepted from [::1]:54978
Cycle 9, connection accepted from [127.0.0.1]:44562
: OK

@dklimkin
Copy link
Author

Is there a way to retrieve tests/test-suite.log?

1. There was a data race on _running
In some cases _sock.down() was called before the last read was
complete. That resulted in fd no longer available exception.
Also, in theory the thread might never see the value flipped.

2. Binding to two sockets and reading from the one that's ready on listener thread.
In some configurations connection gets closed immediately when connection via
IPv4 address. Used the same approach as in API server (it must be there for a reason).
Fixes issue #254.

3. Generating chars as per constructor parameter.

4. Connecting to localhost instead of 127.0.0.1, otherwise the test fails on IPv6-only systems.
@dklimkin
Copy link
Author

Ok, pulled logs:

Test name: tcpstreamtest::baseTest
uncaught exception of type ibrcommon::socket_exception

  • Cannot bind to socket with address [::1]:4343

There seems to be actually a bug with ibrcommon::basesocket::hasSupport() as it reports IPv6 to be available on Travis.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 63.744% when pulling c0ca51f on dklimkin-travis:tcpstreamtest into e7cfb5f on ibrdtn:master.

@morgenroth
Copy link

ibrcommon::basesocket::hasSupport only checks if the system (Kernel) has IPv6 support. It does not tell you if the loopback device has actually an IPv6 address. A simple search would have helped you here.
http://lmgtfy.com/?q=travis+ci+ipv6+loopback

The PR above is a mess. There are unnessary and possibly breaking changes ("size_t" => "int", commented out code, ...) in there. Please clean-up first and rebase your changes. I do not like commits which add stuff if it is removed again by another commit in the same PR.

@dklimkin
Copy link
Author

Correct, this is not ready to merge yet. I only learnt about IPv6 issue on travis upon checks in this PR, so temporary commented out code that causes it and looking for the solution. I actually read all the issues logged for Travis on this but there is no solution so far except to conditional compilation.

Do you think we should go over interfaces and check availability in hasSupport()? The same code is used in ApiServer but it looks like it has no tests coverage so none a broken.

In the meanwhile, can we start a review for the rest of the PR? I'd like to understand what specifically is "messy" here.

  1. Commented out code -- will resolve before merge.
  2. ints -- there were 3 nested loops, using size_t, size_t and int. General recommendation is to use signed types for loop variables, unless it is compared to a size_t in the condition, e.g. due to using sizeof(). As in every case we have pre-defined constants well in range for int, I changed it.
  3. I feel it's good to keep comments and patches resolving comments as a nice history on the PR. If I rebase, some comments will disappear. When merging, you can choose "Squash and merge" to have a nice single commit on the main branch.

@dklimkin dklimkin closed this Apr 11, 2018
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 this pull request may close these issues.

None yet

3 participants