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

TESTS/NETSOCKET: Refinement of `netsocket` tests #5604

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
10 participants
@betzw
Contributor

betzw commented Nov 28, 2017

Description

Refinement of netsocket test socket_sigio

Status

READY

Migrations

NO

Deploy notes

A call to

`TCPSocket::recv(void *data, nsapi_size_t size)`

returns, following the mbed documentation, the number of received bytes on success, and a negative error code on failure.

So in case of success, the return value depends on both the value of parameter size but also on the amount of data already available. This means, that the value returned can be lower than or equal to the size of the data buffer passed as argument to the call.

Therefore, in the cases of test_tcp_hello_world() & find_substring() (i.e. test socket_sigio), the calls to TCPSocket::recv() might return from one byte up to sizeof(buffer) - 1 (i.e. 511) bytes for each single call, while the tests expect to receive the whole response string with a single call.

The commit applies a fix to this situation by implementing a receive loop which exits once there is no data anymore available to be read from the socket.

Notes:

cc @ashok-rao, @MarceloSalazar, @screamerbg

@ashok-rao

This comment has been minimized.

Contributor

ashok-rao commented Nov 28, 2017

@sarahmarshy , could you please check the changes? Thanks!

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Nov 28, 2017

A TCP loop is indeed required. The end condition is ret being 0 (clean end of stream) or negative (error). You should probably fail outright if you see negative return, rather than just terminating the loop.

The set_timeout call is erroneous - in HTTP 1.0 you need to keep reading until end of stream or error, not until you just have no more data for now. Maybe you could have a long give-up timeout, but assuming all the data arrives immediately isn't right. If you've no timeout on the 1st read, there's no clear reason to have any timeout on subsequent reads.

I don't see what the UDP change is achieving. The test is expecting to receive exactly those sizes - those should be the sizes the server is sending, and the point of the test is really about stressing the network with that DTLS-handshake-like 5-packet burst and making sure all those 5 packets are received.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Nov 28, 2017

(In HTTP 1.1 you would terminate when you've received the correct number of total bytes - that would also be a possible TCP termination condition here).

To be clear on UDP sizes - it's a datagram protocol. The datagram size is part of the protocol. Datagram size is determined by the transmitter. If no datagrams are dropped (which is the point of the test), and the buffer is big enough for each datagram (which it is), the 5 sizes returned by recv() must exactly match the 5 sizes passed to send() at the transmit end.

@betzw

This comment has been minimized.

Contributor

betzw commented Nov 30, 2017

From the mbed specs:

Do not see anything about that return of 0 means "clean end of stream" or about "datagram sizes".

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Nov 30, 2017

That does look rather underspecified, but it's intended to be (a subset of) the standard BSD/POSIX socket interface. In the absence of any indication to the contrary, you should assume standard behaviour.

A TCPSocket that did not indicate end of stream or a UDPSocket that did not tell you how big datagrams were would be rather useless.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Nov 30, 2017

Relevant POSIX description here:

http://pubs.opengroup.org/onlinepubs/009695399/functions/recv.html

The recv() function shall return the length of the message written to the buffer pointed to by the buffer argument. For message-based sockets, such as SOCK_DGRAM [...], the entire message shall be read in a single operation. If a message is too long to fit in the supplied buffer [...] the excess bytes shall be discarded. For stream-based sockets, such as SOCK_STREAM, message boundaries shall be ignored. In this case, data shall be returned to the user as soon as it becomes available, and no data shall be discarded.

Upon successful completion, recv() shall return the length of the message in bytes. If no messages are available to be received and the peer has performed an orderly shutdown, recv() shall return 0. Otherwise, -1 shall be returned and errno set to indicate the error.

@betzw

This comment has been minimized.

Contributor

betzw commented Nov 30, 2017

That does look rather underspecified, but it's intended to be (a subset of) the standard BSD/POSIX socket interface. In the absence of any indication to the contrary, you should assume standard behaviour.

A TCPSocket that did not indicate end of stream or a UDPSocket that did not tell you how big datagrams were would be rather useless.

Sorry, but I really do not think that arguing like this is permissible.

@betzw

This comment has been minimized.

Contributor

betzw commented Nov 30, 2017

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Nov 30, 2017

I agree the documentation should be fleshed out - it's not sufficient to just assume those details of POSIX, no matter how fundamental they are to concept of the API.

Some other guys are busy formalising some of that assumed behaviour into stack tests - I'll prod them to make sure they also update the documentation for anything they're testing that isn't stated - can't really fairly fail people for things that aren't stated either directly or by reference. (@SeppoTakalo)

Is that the issue here - there is a (potential?) implementation that is losing datagram boundaries, so fails the "dtls handshake" test? If it fails this test for that reason, it would not support most applications.

We can't expect every stack to function exactly the same - they will have quirks, particularly in event signalling - but they at least have to be able to send and receive TCP and UDP data with the correct semantics, or you can't write correct applications.

@betzw betzw force-pushed the betzw:betzw_tests_netsocket_wb branch from 6f34221 to 96c569e Nov 30, 2017

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Nov 30, 2017

@betzw Thanks for pointing out the missing documentation.

I added little bit of clarification here: #5623

@betzw

This comment has been minimized.

Contributor

betzw commented Dec 14, 2017

I have just removed the problematic modifications as contested by @kjbracey-arm.
Pls. give it a chance.

@kjbracey-arm

This is an worthwhile improvement - makes the test cope if data doesn't arrive in one recv call.

The test does have other existing issues, particularly that it uses blocking sockets with SIGIO, which is weird, but can't expect this PR to fix everything.

while((ret = sock->recv(buffer+len, sizeof(buffer) - 1 - len)) > 0) {
len += ret;
}
if(len >= 0)

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 14, 2017

Contributor

Don't need this if - len can't be negative if you're only adding positive numbers to it. (And if it could be negative here, I think find_substring would explode below anyway).

@betzw betzw force-pushed the betzw:betzw_tests_netsocket_wb branch from da3d329 to 48704a7 Dec 14, 2017

@betzw

This comment has been minimized.

Contributor

betzw commented Dec 14, 2017

I think I have fixed the if(len >= 0) issue.

makes the test cope if data doesn't arrive in one recv call.

Regarding this, I would it expect to behave correctly considering what you said in one of your comments before about orderly shutdown and HTTP.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Dec 14, 2017

Yes, the code will now correctly pass any valid return sequence from recv, as far as I can see.

The remaining pre-existing issues are about spotting stuff that should really be failed, eg error return after receiving "Hello world", or trailing data. I'd also rather the sigio test didn't use blocking sockets - means it will just go straight into blocking state after first sigio.

But this PR doesn't need to deal with those.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Dec 18, 2017

@0xc0170 0xc0170 changed the title from [TESTS/NETSOCKET] Refinement of `netsocket` tests to TESTS/NETSOCKET: Refinement of `netsocket` tests Dec 18, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 18, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 18, 2017

Build : SUCCESS

Build number : 715
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5604/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 18, 2017

Please review the latest tests results, related to this changeset

@0xc0170 0xc0170 added the needs: work label Dec 18, 2017

@cmonr cmonr removed the needs: review label Feb 2, 2018

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Feb 7, 2018

K64F runs out of event queue entries, since sigio calls get_data() several times while blocking for recv(). Updating event queue entries from 4 to 10 to test_socket_attach() should correct the issue:
EventQueue queue(10*EVENTS_EVENT_SIZE);

@betzw

This comment has been minimized.

Contributor

betzw commented Feb 7, 2018

Suggestion applied in latest commit.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 7, 2018

Thanks !
Is the last commit merge with upstream master ? Can you rebase instead please?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Feb 7, 2018

It would be a little more solid to ensure you never queue the event multiple times. Not sure if there's a way to do that built-in to the Event stuff? Feels like there should be. I've seen people use this sort of model manually:

void event_processor() {
     event_pending = false;
     // do work;
}

void irq_handler() {
    if (!event_pending) {
        event_pending = true;
        event.call(xxx);
    }
}
@betzw

This comment has been minimized.

Contributor

betzw commented Feb 7, 2018

Is the last commit merge with upstream master ? Can you rebase instead please?

Should be.

It would be a little more solid to ensure you never queue the event multiple times. Not sure if there's a way to do that built-in to the Event stuff? Feels like there should be. I've seen people use this sort of model manually

Pls. feel free to adapt any solution you want ...

@betzw betzw force-pushed the betzw:betzw_tests_netsocket_wb branch from 23ce7db to b8ba341 Feb 7, 2018

@betzw betzw force-pushed the betzw:betzw_tests_netsocket_wb branch from b8ba341 to d6cb385 Feb 7, 2018

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Feb 7, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 7, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 7, 2018

Build : SUCCESS

Build number : 1083
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5604/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 merged commit 04f0f2b into ARMmbed:master Feb 8, 2018

19 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mbed-ci-generic Build finished.
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Feb 8, 2018

@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 9, 2018

OK this one should not have gone in as it stood. The giveaway is the last commit :
d6cb385
Was a merge commit :
416 changed files with 154,614 additions and 2,707 deletions.
Should have been a rebase

@betzw

This comment has been minimized.

Contributor

betzw commented Feb 12, 2018

The commit was of two commits: 8ec9f6b & d6cb385.
Hope this helps.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Feb 12, 2018

The merge commit is so convoluted that it is not easily patchable to the release branch, thus this is being bumped out to 5.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment