Pull requests

#21 Declined

New event-based love.network module built with Boost.Asio and ENET

  1. John-John Tedro


I started working on a native love.network module that in the end should be able to replace luasocket for all client related network activities. This rose from some frustration I had getting network support into my own game using luasocket as it looks today.

I've introduced a new type hierarchy under src/modules/network including the following classes.

  • love::network::Network (Abstract Module)
  • love::network::Socket (Abstract Object)
  • love::network::TcpSocket (Abstract Object)
  • love::network::asio::Network (Module)
  • love::network::asio::TcpSocket (Object)

The abstract parts are generic enough to be reusable for other implementations, I've gone through some hoops to make sure that Boost.Asio is not a strict dependency for a network module implementation, but I would strongly opt for it since it makes everything a whole lot easier. Sockets carry (void*) user data which is used to store references to the lua socket objects for the purpose of invoking the callbacks.

The network engine currently runs in it's own thread NetworkThread and pushes events using synchronized queues through the love.network.pump call.

There is also a minor change to runtime.cpp that sets the meta-variable __newindex on newtype to allow for assignment of new keys to the resulting userdata. This is a feature I use to implement the onRead and onStateChange callbacks.

The Network type also has it's own set of event queues and pump, because integrating against the existing Event module didn't seem like a good fit.

Please have a look. I don't assume you will merge it anytime soon but will appreciate further feedback.

  • Learn about pull requests

Comments (3)

  1. rude repo owner

    As far as I can tell, this is pretty nice, especially that you bothered to abstractify the module. Looks like it will integrate nicely with our existing code. :)

    However, there are some issues (of varying magnitude):

    • I generally hate Boost. Boost is a monolithic monster who feeds of the pain and misery of others. And if you think my personal childish feelings shouldn't get in the way of progress, you are very much mistaken. (OK, Boost usage in this case doesn't look that bad, but I have to prove to myself that this dependency won't be a giant pain in the ass on platforms with shitty/no package managers, i.e. Windows and Mac). It's tempting to name-drop SDL_net at this point, but of course I haven't really used SDL_net.
    • The "Tcp" is an unpronounceable abbreviation, so surely it should be TCPSocket. Unless we already have a public type which establishes the Tcp convention, and I sure hope we don't.
    • You implement a new pattern here, and that is callbacks as fields on native types. When you implement a new pattern, expect resistance. :) We don't have many callbacks in our code currently, but there are indeed some (e.g. love.physics), and they use this pattern: love.network.newTCPSocket(stateCallback, dataCallback). In other words, the callbacks are passed as parameters to functions. So I recommend doing that, even if you can find minor reasons for your current approach being better.
    • You implement a separate event system. I don't know if this is OK. Probably. It could certainly be argued that our current event system is kind of monolithic/lame. Whatever we do here, we have to make sure that it's not completely incompatible with what we intend to do in the future, if anything.
    • Your indentation seems to be messed up in some places. (E.g. w_pump). We use tabs for indentation.
    • We normally don't underscore private variable/function names.
    • We normally put the { in control statements on a separate line.
    • We always put the asterisk next to the variable name. (E.g. Foo *foo, not Foo* foo).
    • http://love2d.org/wiki/Code_Style

    I consider network code firmly in Bartbes territory, so we also have to wait and see what he has to say about this. Bartbes also had plans of making a network module himself, so he may pretend to hate your code altogether just to do it (or not do it, like me) himself. :D

  2. John-John Tedro author

    Cleaned up as much as I could find according to the style guide, apart from that, the following concerns still seem valid.

    • The use of Boost
    • A separate event system

    Regarding boost, the only component I really have had problems compiling for other architectures is Boost.Thread, the only link dependency required for asio are Boost.System (which is a small library and can be omitted) and headers unless we want to have ssl with asio. I am not even going to argue for the other uses of boost (BOOST_FOREACH, boost::array), lets just say it increases my productivity and can be trivially replaced with more plumbing, plus foreach and array support is up and coming with c++11.

    Regarding the event system, the existing one IMO needs more generalization before it can be migrated and used by modules outside of event. However when this happens I expect that the changes to love.network will be minimal (keep the current interfaces for pushing messages through network, drop the pump implementation and push the messages to the general event queue instead).

    On a side note, I am currently working with adding enet and udp support into this, so the complete interfaces would be new{TCP,UDP,ENET}Socket with similar interfaces.

    I am looking forward to Bartbes comments. live long and prosper.