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

[FR] Allow a callback set by srt_listen_callback to be removed #2634

Closed
Chardrazle opened this issue Jan 27, 2023 · 4 comments
Closed

[FR] Allow a callback set by srt_listen_callback to be removed #2634

Chardrazle opened this issue Jan 27, 2023 · 4 comments
Labels
Type: Enhancement Indicates new feature requests

Comments

@Chardrazle
Copy link

There may be cases where the "opaque" user-pointer value that is passed to srt_listen_callback is associated with some resources.
If an RAII style approach is being taken then ideally there is a symmetry to the set up. So the callback would be removed as a reverse operation to the addition.
However, when calling srt_listen_callback with a nullptr for the callback function the call is rejected as unsupported.

This appears to suggest that (once set) the callback has to exist as long as the socket. It puts the emphasis on the client to manage the state in the callback, as opposed to just switching it off.

Could the srt_listen_callback support reset via nullptr?
Presumably some internal state/thread synchronisation management may be the reason this is not currently available?

@Chardrazle Chardrazle added the Type: Enhancement Indicates new feature requests label Jan 27, 2023
@ethouris
Copy link
Collaborator

There could be quite a problem with having the listener callback removed without making sure first that the previously set listener callback isn't currently being executed. I can hardly imagine any logics standing behind a procedure when your socket is open for any incoming connection, about which you have no idea when they would happen, and after having 5 new connections coming in you let in 2 with the execution of the listener callback and then the last 3 without.

Note that usually - although this somehow wasn't exactly intended (maybe this check should be added) - you install the listener callback before setting up the listening state by srt_listen or otherwise there could be a similar situation as above described. But if this call could be rejected after calling srt_listen, then this is a part of the socket setup, which cannot be reverted. The state transition is: CREATED -> BOUND -> CONNECTED/LISTENING -> BROKEN -> CLOSED. There's no way of getting back to any previous state. In the case of the listener socket it means also that once the socket listens, it cannot "unlisten".

So if you really need by some reason to continue listening on the given port, but stop using the listener callback on that bindpoint, you can currently do it by creating a new listener socket and close the old one. This means that some connections that are pending at this very moment, these will be rejected, but once the operation is done, all further connections will be accepted, and processed without the listener callback previously installed. Note the order though:

  1. Create the new socket to replace this listener (srt_create_socket)
  2. Bind this socket to the same endpoint as the listener (srt_bind). Note that the listener should not change the SRTO_REUSEADDR option from the default true. This will make the listener socket's endpoint shared (together with internal socket resources) and prevent the listener from being misused at the moment. The socket probably should be configured additionally, if needed - for example, you might want to set some special password, should your listener callback set up a password basing on some information from the calling party.
  3. Close the current listener socket (srt_close).
  4. Set the newly created socket listen (srt_listen) and use it in place of the old listener.

For me it sounds more reasonable though that the time gap between closing the old listener and setting listen on the new one should be even intentionally extended, not minimized. But as I said, I don't exactly understand the logics of the application that might want to do such a thing.

@Chardrazle
Copy link
Author

Fair enough, I suspected it would be more complicated underneath.

There may be an inconsistency - given that nullptr cannot be set. The srt_listen_callback function does not return an error if called with a different callback function. This does replace the previous one. Surely this would suffer from the same symptoms you describe?

I'm also curious about a point you made. I am currently setting the callback before calling srt_listen. I assumed this would be correct in case the srt_listen immediately went off and asynchronously handled something - even if it wasn't serviced until the epoll event. The callback is being invoked, so it seems to work ok.
Is this the incorrect order?

@ethouris
Copy link
Collaborator

This is the correct order and exactly the order I described - though I can see now that the listener callback function lacks some API guards and more than I saw above. Thanks for pointing me this.

Note that the listener "starts working" exactly at the moment after you call srt_listen - since this moment every incoming connection will be handled, and the listener callback, if installed, will be called. When the application calls srt_accept it will only get the already prepared connected socket, but everything else for the incoming connection is already done.

@ethouris
Copy link
Collaborator

ethouris commented Mar 7, 2023

The inconsistency is resolved in #2683; closing this one. Please reopen if this doesn't match the conditions.

@ethouris ethouris closed this as completed Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Indicates new feature requests
Projects
None yet
Development

No branches or pull requests

2 participants