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

Added getSocketDescriptor() to http::server #140

Merged
merged 1 commit into from
Apr 7, 2016
Merged

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented Apr 5, 2016

No description provided.

@eile
Copy link
Contributor

eile commented Apr 6, 2016

+1

@@ -310,6 +310,18 @@ const servus::URI& Server::getURI() const
return _impl->uri.toServusURI();
}

SocketDescriptor Server::getSocketDescriptor() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning a low-level fd from the Server seems fishy to me.

Copy link
Author

Choose a reason for hiding this comment

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

Having access to the socket handle is very useful in conjunction with a QSocketNotifier. It handles the poll() as part of the QEventLoop and emits an activated() QSignal when the socket has new data available. This signal can be connected to a lamba which calls recieive(), so with a few lines of code you have a server which automatically receives without the need for a separate processing thread. I see no easy alternative to that mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally understand it. But still looks fishy :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a commonly used pattern. You get an opaque handle to poll, but should not use it for something else. ZMQ provides it, X11 ConnectionNumber does, and many more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not disagree, but it feels like there should be extendable layer around it. A socket and poll classes. In C world it is more understandable but in C++ world these can be wrapped nicer.

@rdumusc rdumusc merged commit fd2a814 into HBPVIS:master Apr 7, 2016
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.

3 participants