Skip to content

[ONME-2844] Supporting non-blocking connect() #3457

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 23, 2016
Merged

[ONME-2844] Supporting non-blocking connect() #3457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 44 additions & 9 deletions features/netsocket/TCPSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,47 @@ nsapi_error_t TCPSocket::connect(const SocketAddress &address)
_lock.lock();
nsapi_error_t ret;

if (!_socket) {
ret = NSAPI_ERROR_NO_SOCKET;
} else {
// If this assert is hit then there are two threads
// performing a send at the same time which is undefined
// behavior
MBED_ASSERT(!_write_in_progress);
_write_in_progress = true;

bool blocking_connect_in_progress = false;

while (true) {
if (!_socket) {
ret = NSAPI_ERROR_NO_SOCKET;
break;
}

_pending = 0;
ret = _stack->socket_connect(_socket, address);
if ((_timeout == 0) || !(ret == NSAPI_ERROR_IN_PROGRESS || ret == NSAPI_ERROR_ALREADY)) {
break;
} else {
blocking_connect_in_progress = true;

int32_t count;

// Release lock before blocking so other threads
// accessing this object aren't blocked
_lock.unlock();
count = _write_sem.wait(_timeout);
_lock.lock();

if (count < 1) {
// Semaphore wait timed out so break out and return
break;
}
}
}

_write_in_progress = false;

/* Non-blocking connect gives "EISCONN" once done - convert to OK for blocking mode if we became connected during this call */
if (ret == NSAPI_ERROR_IS_CONNECTED && blocking_connect_in_progress) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check just be ret == NSAPI_ERROR_IS_CONNECTED && _timeout > 0? What if a socket returns NSAPI_ERROR_IS_CONNECTED on the first call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that, I just realized what that implies : )

ret = NSAPI_ERROR_OK;
}

_lock.unlock();
Expand Down Expand Up @@ -81,9 +118,8 @@ nsapi_size_or_error_t TCPSocket::send(const void *data, nsapi_size_t size)
}

_pending = 0;
nsapi_size_or_error_t sent = _stack->socket_send(_socket, data, size);
if ((0 == _timeout) || (NSAPI_ERROR_WOULD_BLOCK != sent)) {
ret = sent;
ret = _stack->socket_send(_socket, data, size);
if ((_timeout == 0) || (ret != NSAPI_ERROR_WOULD_BLOCK)) {
break;
} else {
int32_t count;
Expand Down Expand Up @@ -125,9 +161,8 @@ nsapi_size_or_error_t TCPSocket::recv(void *data, nsapi_size_t size)
}

_pending = 0;
nsapi_size_or_error_t recv = _stack->socket_recv(_socket, data, size);
if ((0 == _timeout) || (NSAPI_ERROR_WOULD_BLOCK != recv)) {
ret = recv;
ret = _stack->socket_recv(_socket, data, size);
if ((_timeout == 0) || (ret != NSAPI_ERROR_WOULD_BLOCK)) {
break;
} else {
int32_t count;
Expand Down
3 changes: 3 additions & 0 deletions features/netsocket/nsapi_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ enum nsapi_error {
NSAPI_ERROR_DHCP_FAILURE = -3010, /*!< DHCP failed to complete successfully */
NSAPI_ERROR_AUTH_FAILURE = -3011, /*!< connection to access point failed */
NSAPI_ERROR_DEVICE_ERROR = -3012, /*!< failure interfacing with the network processor */
NSAPI_ERROR_IN_PROGRESS = -3013, /*!< operation (eg connect) in progress */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the distinction between NSAPI_ERROR_IN_PROGRESS and NSAPI_ERROR_WOULD_BLOCK?

Copy link
Contributor

@kjbracey kjbracey Dec 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the distinctions seem to be:

"would block" - "that call had no effect, but you can try the same thing again later and it might work"
"in progress"- "that call started something, but it hasn't finished, check back for status later"

In the case of "connect", checking back after "in progress" could either be trying connect again (in which case you can expect "already in progress" or "is connected" errors), or just going ahead and trying to read/write (in which case you could get "not connected").

So "would block" means you have to try the same call again. "In progress" means you could just start trying the next call, as it will be accepted once the in-progress thing finishes (or maybe it'll even get queued).

Distinction doesn't actually matter to the blocking abstraction here though, as it does just keep calling connect().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having written that, I guess it follows that the blocking loop here could respond to "would block" by retrying. But POSIX doesn't have "would block" as a possible connect() return, and I guess this shouldn't encourage that.

Also, only interpreting new codes has the handy effect of ensuring this patch doesn't change existing behaviour for any current stacks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, that makes sense. Thanks for clarifying.

Out of curiousity, is it valid to go straight to a non-blocking send loop after starting a non-blocking connect? Would send return NSAPI_ERROR_WOULD_BLOCK or NSAPI_ERROR_NO_CONNECTION at that point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be sort of be fine. BSD/POSIX would return ENOTCONN, so I'd expect stacks to give NO_CONNECTION to match.

Unfortunately in NSAPI, NSAPI will also give you NO_CONNECTION in place of ECONNFAILED, ECONNRESET or ETIMEDOUT. So you wouldn't be able to distinguish between a failed connection and one which just hadn't completed yet.

I guess you have to keep calling connect(), like this blocking loop.

And to make that work, you also have to make sure you do get a sensible error return from connect() after failure. What you wouldn't want is for it to give up on a connection in the background, and then have the next connect call just start it again.

That's generally handled by one or both of - having sticky errors on sockets, so next socket call after an error returns the error code, and/or not permitting more than 1 connect() attempt on any socket. Nanostack does the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update - apparently network stacks are not as consistent as I thought on this. BSD 4.4 actually doesn't return the sticky error code on connect(), and does permit a second connect, so this loop wouldn't work plugged raw into BSD.

https://stackoverflow.com/questions/17769964/linux-sockets-non-blocking-connect
https://cr.yp.to/docs/connect.html

Ew. Well, we can demand that underlying stacks handle this if they're supplying non-blocking connect. Doesn't seem unreasonable. They could incorporate their own sticky error handling or equivalent event handling, or call getsockopt(SO_ERROR) first in their connect handler.

NSAPI_ERROR_ALREADY = -3014, /*!< operation (eg connect) already in progress */
NSAPI_ERROR_IS_CONNECTED = -3015, /*!< socket is already connected */
};

/** Type used to represent error codes
Expand Down