Page MenuHomePhabricator

Add Socket::Create factory method which uses socket protocol to find an appropriate implementation class.
ClosedPublic

Authored by ovyalov on Oct 26 2015, 11:12 AM.

Details

Reviewers
zturner
clayborg
Summary

Add Socket::Create factory method which uses socket protocol to find an appropriate implementation class.

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 38437.Oct 26 2015, 11:12 AM
ovyalov retitled this revision from to Make Socket to support plugin interface.
ovyalov updated this object.
ovyalov added a reviewer: clayborg.
ovyalov added a subscriber: lldb-commits.
zturner requested changes to this revision.Oct 26 2015, 11:29 AM
zturner added a reviewer: zturner.
zturner added a subscriber: zturner.

I'm not sure I agree with this change. The interface to programming with socket is by definition a property of the Host operating system. It seems to me like Host was already the correct place for this code.

This revision now requires changes to proceed.Oct 26 2015, 11:29 AM

I'm not sure I agree with this change. The interface to programming with socket is by definition a property of the Host operating system. It seems to me like Host was already the correct place for this code.

Base Socket class which is utilized by users remains in Host interface - moving implementation classes into Plugins somewhat simplifies creation/registration process with PluginManager (instead of if (scheme=="tcp"){} else if (scheme=="unix"){}..).

I still think implementation should be in Host as well. If the idea is to simplify the creation scheme, then you could create an enum:

enum class SocketType {
    Tcp,
    Udp,
    UnixDomain
};

and provide a static method on SocketBase like this:

class SocketBase {
public:
    static std::unique_ptr<SocketBase>
    MakeSocket(SocketType type);
};

Although this is in the long term, eventually we will require a modules build of LLDB, so introducing more circular library dependencies should really be avoided, as they are incompatible with modules build and will have to be refactored again once we try to get a modules build working.

Let's see what Greg and others say, but personally I don't think the plugin system should be used solely to gain access to a factory-like creation interface.

clayborg edited edge metadata.Oct 26 2015, 1:06 PM

The plug-in interface I was thinking of was the Connection class. This would allow different platforms to support different Connection subclasses for things like network, serial, USB, Firewire, shared memory, IPC and others. Not sure if socket is stand alone enough to warrant its own plug-in interface. How are the socket subclasses used around LLDB? Are they used directly? Or are they place into a Connection class?

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

Definitely not.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

Agreed. If they are mostly used in connection classes, I would say to do what you said to do: add an enum and factory static method on Socket that uses that enumeration and then make Connection class into plug-ins they themselves vend the URL prefixes and instantiate a connection class with any arguments. So:

1 - convert Socket class to use enum + factory class:

enum class SocketType {
    Tcp,
    Udp,
    UnixDomain
};

class SocketBase {
public:
    static std::unique_ptr<SocketBase>
    MakeSocket(SocketType type);
};

2 - Make Connection classes into plug-ins and have them parse the URLs in the "static Connection *Connection::GetConnectionForURL(const char *url);"

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

That is fine and the above two step solution would enforce that.

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

There are some corner cases that don't fit into regular model:

  • Creation of UDP connection - LLDB uses pair of send/receive sockets and that's why we need to use UDPSocket::Connect instead of using factory method.
  • ConnectionFileDescriptor::Connect creates a new TCP socket without giving up the ownership of a socket handle.

As an option, we can introduce static std::unique_ptr<Socket> Socket::Create(const char* scheme, bool child_processes_inherit, Error& error); as socket class creator method but without moving socket classes into plugins.

ovyalov updated this revision to Diff 38484.Oct 26 2015, 5:06 PM
ovyalov retitled this revision from Make Socket to support plugin interface to Add Socket::Create factory method which uses socket scheme (tcp, unix,..) to find an appropriate implementation class..
ovyalov updated this object.
ovyalov edited edge metadata.
clayborg accepted this revision.Oct 26 2015, 5:09 PM
clayborg edited edge metadata.

Do we need this GetScheme() method? And instead of having Create take a string, couldn't it just take a SocketProtocol instead? It seems like the logic of converting the string to the enum should be done in the Acceptor class, as this URI format is defined by the acceptor, and the socket itself is just a low-level host layer abstraction.

ovyalov updated this revision to Diff 38487.Oct 26 2015, 6:20 PM
ovyalov retitled this revision from Add Socket::Create factory method which uses socket scheme (tcp, unix,..) to find an appropriate implementation class. to Add Socket::Create factory method which uses socket protocol to find an appropriate implementation class..
ovyalov updated this object.
ovyalov edited edge metadata.

Thanks for suggestions - addressed review comments.

zturner accepted this revision.Oct 26 2015, 7:19 PM
zturner edited edge metadata.
This revision is now accepted and ready to land.Oct 26 2015, 7:19 PM
ovyalov closed this revision.Oct 27 2015, 10:34 AM

Files:

/lldb/trunk/include/lldb/Host/Socket.h
/lldb/trunk/include/lldb/Host/common/UDPSocket.h
/lldb/trunk/source/Host/common/Socket.cpp
/lldb/trunk/tools/lldb-server/Acceptor.cpp
/lldb/trunk/tools/lldb-server/Acceptor.h

Users:

ovyalov (Author)

http://reviews.llvm.org/rL251417