Skip to content

Added timeout reason. Updated correctly reject reason on timeout#1194

Merged
maxsharabayko merged 4 commits into
Haivision:masterfrom
ethouris:dev-connect-nonblocking-error
May 19, 2020
Merged

Added timeout reason. Updated correctly reject reason on timeout#1194
maxsharabayko merged 4 commits into
Haivision:masterfrom
ethouris:dev-connect-nonblocking-error

Conversation

@ethouris
Copy link
Copy Markdown
Collaborator

Fixes #1164

@ethouris ethouris added Priority: Medium Type: Maintenance Work required to maintain or clean up the code [apps] Area: Test applications related improvements [core] Area: Changes in SRT library core Impact: Significant labels Mar 20, 2020
@ethouris ethouris added this to the v1.5.0 milestone Mar 20, 2020
@ethouris ethouris requested a review from maxsharabayko March 20, 2020 16:13
@ethouris ethouris self-assigned this Mar 20, 2020
@maxsharabayko
Copy link
Copy Markdown
Collaborator

It does not look right to mix connection rejection reason and connection timeout.
Although I see it is easier to implement.
How would you estimate the corectness and effortt to do the same via srt_getlasterror(..)?

@ethouris
Copy link
Copy Markdown
Collaborator Author

It's not possible to be done via srt_getlasterror because it remembers it in a thread-specific storage, not for a socket-specific storage.

You'd need to add a new field that will remember the operation status in the socket. The call to APIError will have to store this value somehow in both places so that it can be later retrieved by a new function, kinda srt_getlasterror_in(socket_id). So should function that perform the connection in the background.

For implementation in the groups I don't even have an idea - especially that the same id can be simultaneously used for sending data and connecting a new link, which is also done in the background, so theoretically an error on the group for connecting would have to be stored differently than any other error, or we'd have to make a very specialized function that would be used exclusively to get the connection errors in order to maintain interchangeability between sockets and groups in the API.

@maxsharabayko
Copy link
Copy Markdown
Collaborator

maxsharabayko commented May 12, 2020

The problem this PR is addressing is getting the reason for a non-blocking connection to fail.

  1. A non-blocking srt_connect(..) returns immediately. It reports only errors due to incorrectly specified parameters.

  2. Call srt_epoll_wait(..) to get notified of the connection state.
    If there was a connection timeout, the error is reported in read and write FDs.

Calling srt_getlasterror(nullptr) returns a thread-local error, not an error for the socket.

This PR reuses srt_getrejectreason(socket_id) to get connection timeout error as well.

Copy link
Copy Markdown
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Requested changes

@ethouris ethouris requested a review from stevomatthews May 12, 2020 12:31
@maxsharabayko maxsharabayko mentioned this pull request May 12, 2020
33 tasks
Comment thread docs/API-functions.md Outdated
Comment thread docs/API-functions.md Outdated
Comment thread docs/API-functions.md Outdated
Copy link
Copy Markdown
Collaborator

@stevomatthews stevomatthews left a comment

Choose a reason for hiding this comment

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

Minor edits

ethouris and others added 2 commits May 13, 2020 09:51
(post-fix pending)

Co-authored-by: stevomatthews <smatthews@haivision.com>
@maxsharabayko
Copy link
Copy Markdown
Collaborator

The documentation should also provide a guideline for when to call srt_getrejectionreason(..) in a non-blocking mode.
It does this for a blocking mode only as of now.
srt_epoll_wait(..) errors if you waited for a connection. Then get reject reason.
What would be the workflow for rejection reason if epoll waits both for conection events, and reading events, if this workflow is the correct one. 🤔

@maxsharabayko maxsharabayko merged commit 5ffcd7c into Haivision:master May 19, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[apps] Area: Test applications related improvements [core] Area: Changes in SRT library core Priority: Medium Type: Maintenance Work required to maintain or clean up the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MAINT] The rejection process with the non-blocking connecting socket should be improved

4 participants