Asynchronous Design

[computerquip]

I've been working on a asynchronous network library meant to be used with the Teamspeak 3 remote server query. It will serve various purposes but for now, it's only meant to parse messages, send an event when a message is received, and provide a method of sending messages in a convenient manner. There are a few problems.

1. I'm not sure how to handle errors in an asyncronous environment. Any suggestions on this would be lovely. The environment will optionally use threads but they are not required.

2. I have four functions (that deal with actual networking):

 extern "C" {

void* ts3qclient_initialize(struct ts3qsEvent_t* events);
int ts3qclient_connect(void* handle, char const* server, unsigned short port);
void ts3qclient_quit(void* handle);	
void ts3qclient_sendCommand(void* handle, char const* command);

}

The initialize function will be provided all the events you wish to participate in. Unfortunately, the structure you pass must obviously be initialized to nothing if you don't plan on using all events.

The connect function will obviously connect. Password and authentication is sent via sendCommand which is why its not handled here.

A serious issue is of course the lack of error handling and the confusing return codes. I'm open for suggestions on what would be most useful to return. Please take note that in an asynchronous environment, you can't really have networking functions return error codes since the result of the asynchronous action hasn't actually been accomplished yet!

Summary
I really need some feedback. Please keep it constructive. The interface is in C because it would be sinful to complicate ABI over four functions. I posted in lounge simply because it has more to do with design than code.

[Luc Lieber]

Generally when using an asynchronous pattern, one would provide a handler, or handler chain, that are fired when certain events happen. (The following is in pseudo-C++, but can be easily enough changed to C). The examples below are also pretty dumbed down to only show the concept.

/// the sole argument is true if the connection was successful, otherwise false
typedef std::function<void(bool)> ConnectionHandler;

void connect(ConnectionArgs args, ConnectionHandler handler)
{
  // asynchronously connect and post the handler to a main thread
}

typedef std::function<void(const ErrorCode&, int)> WriteCompletionHandler;

void write(BufferSequence sequence, WriteCompletionHandler handler)
{
 // asynchronously write the message
 // post the handler to be fired on a 'main thread'
 // eg. mainThread.post(std::bind(handler, error, bytesWritten));
}

void someFunction(BufferSequence sequence)
{
  write(
    sequence,
    /// To be fired on a thread that's polling for events
    [&](const ErrorCode& error, int bytesWritten)
    {
      if(error != OPERATION_SUCCESS)
      {
        //handle error appropriately
      }
      else
      {
        // all is well
      }
    });
}

Asynchronous networking is one of my favorite topics, so let me know if you have any other questions.

It's also important to note that the complexity of an asynchronous design rises considerably. It's all too easy to set up a race-condition-to-be if even a small aspect is overlooked.

[computerquip]

The function void* ts3qclient_initialize(struct ts3qsEvent_t* events); would take a struct that defines a function pointer to be assigned for each provided event. It would look something like this:

struct ts3qsEvent_t {
    void (*event_onConnect)(/*something*/);
    void (*event_onQuit)(/*something*/);      
};

This is fine and dandy. However, would it be appropriate for me to pass some sort of error information (perhaps an integer that can be checked for non-zero)?

Also, I'm not aware if the four functions are adequate. It appears to be adequate to me but I'm not sure of what other features people might want. I'll eventually add some sort of state that controls how the library handles networking (such as timeouts) but I'm unsure if some would find this restrictive somehow.

Also, what should the asyncronous functions return if anything?

[chrisname]

@computerquip
I would do something like this:

struct ts3qsEventArgs {
    // ...
};

typedef void(* ts3qEventHandler)(ts3qsEventArgs* args);

struct ts3qsEventQueue {
    ts3qEventHandler* handlers;
    size_t numHandlers;
};

enum ts3qEventType {
    TS3Q_EVENT_ONCONNECT,
    TS3Q_EVENT_ONQUIT
};

struct /* something */ {
    // ...
    struct ts3qsEventQueue* queue; // one element per member of ts3qEventType
    // ...
};

That way you can have as many handlers for each event as memory allows.

[Luc Lieber]

would it be appropriate for me to pass some sort of error information (perhaps an integer that can be checked for non-zero)?

There isn't really any other way that I can think of when dealing with asynchronous operations other than atomic global state variables (or maybe thread-local storage...). I'd sooner use an enumeration than raw integers though, but that's just a matter of taste. Passing the error state to the event handler seems to be the best way (that is, the way least likely to cause problems) in my opinion.

Also, I'm not aware if the four functions are adequate. It appears to be adequate to me but I'm not sure of what other features people might want. I'll eventually add some sort of state that controls how the library handles networking (such as timeouts) but I'm unsure if some would find this restrictive somehow.

Your public interface doesn't necessarily need to be limited to those four functions. You could politely push those four on your end users while maintaining an 'advanced' set of functions. I'd assume that your current public interface could probably just pass defaults to the 'advanced' functions. I.E.

const unsigned int DEFAULT_TIMEOUT_MILLIS = 5000;
ConnectionHandler DEFAULT_CONNECTION_HANDLER = ?;

void connect(ConnectionArgs args, unsigned int timeout, ConnectionHandler handler)
{
   /// do it
}

void connect(ConnectionArgs args, unsigned int timeout)
{
    connect(args, timeout, DEFAULT_CONNECTION_HANDLER);
}

void connect(ConnectionArgs args)
{
    connect(args, DEFAULT_TIMEOUT_MILLIS);
}

Also, what should the asyncronous functions return if anything?

I would assume that in your case returning void would be just fine. Think of it as little more than a pass-off between threads of execution. You might even want to think about whether or not there are any asynchronous workers that are even capable of accepting the task as well...in order to not block at all, you will need to return instantly in all cases, including the case that no workers can accept your task (such as if all of their work-queues are full). In that case, you might want to return a bool (or the equivalent) to indicate whether the task was successfully submitted or not.

[computerquip]

@chrisname, I see no reason not to add a queue system and this would enable foreign code to be added (i.e. plugins) in a much more convenient manner.

However, I don't see the reason for the enum or for trying to make all events into one generic event (if that's what you mean). I've already tried this and it not only doesn't work well with this protocol, it doesn't seem very efficient.

Take into consideration how I parse everything. It's much easier, although a ton more verbose, to parse each individual message following its specific form. Even then, I've actually tried the former and implemented a parser for it here: http://github.com/computerquip/Teamspeak-Query-handler/blob/master/parser/grammar.y

I stopped trying to add on to this simply because it was ugly and frustrating. Trying to figure out how to parse one rule, which the Teamspeak 3 protocol probably doesn't strictly follow, without breaking yet another rule was very difficult. I also did horribly trying to design a generic method for the user to query commands, options, and values. The method I attempted can be found here: http://github.com/computerquip/Teamspeak-Query-Handler/blob/master/main.cpp

The point is that when I parse like this, I already know what type of message it is by the time I'm done parsing it, making it pointless to make the user figure out what type of message it is.

So, instead of a generic parser, I'm now going to try and parse each individual message: http://github.com/computerquip/qpNetwork/blob/master/parser/teamspeak3/cqParse.lem

[chrisname]

Well, the queue was the main point of my post. I personally would then use an array of event queues with the enum values as indexes, like so:

void call_event_handlers(event_type_t event, const event_args_t* args)
{
    int i;
    for (i = 0; i < library_context.event_queue[event].count; ++i)
        library_context.event_queue[event][i].handler(args); // Handlers can cast args back to desired structure pointer
}

int connect()
{
    on_connect_event_args_t* args;
    call_event_handlers(EVENT_ON_CONNECT, (event_args_t*)args); // Cast to generic structure pointer
}

which will call all the event handlers for the given event with the given arguments.

[computerquip]

I was thinking of using boost signal instead. I also don't see why I can't just populate a struct with unique names per unique event. So a call would look like this:

int connect(){ 
    ...
    pClient->m_Events.sigOnConnect(arg1, arg2, arg3, so on);
}

I hope I wasn't giving the impression that I was only using C in the implementation. I'm only giving a C interface, I don't care what language is used in the implementation.

My only real concern would be the mass of events, although, an array of structures wouldn't be much more ideal anyways. I'd need a structure or signal for 85% of the commands in the following document:

http://media.teamspeak.com/ts3_literature/TeamSpeak%203%20Server%20Query%20Manual.pdf

I do like your idea but I'm not sure which is better as far as efficiency or flexibility goes. :/

[chrisname]

computerquip wrote:

I was thinking of using boost signal instead.

I don't know anywhere near as much about boost as I should.

I also don't see why I can't just populate a struct with unique names per unique event

You can, but using enum values with a generic function that calls all the handlers for a given event rather than having one function per event will result in less code. Your sigOnConnect() function is going to look similar to my call_event_handlers function, but with the downside that it only applies to the "connect" event, whereas the generic solution applies to every event. Using enum is the easiest way to write a generic event handler calling function.

I hope I wasn't giving the impression that I was only using C in the implementation. I'm only giving a C interface, I don't care what language is used in the implementation.

Well, the code is going to be similar in most languages. C++ would be easier and less bug-prone without sacrificing speed or memory, since you could use an STL container instead of writing your own.

I do like your idea but I'm not sure which is better as far as efficiency or flexibility goes. :/

I'd definitely say using an enum is more flexible. If you write your initialisation function in such a way that it takes the value of the last member of your event enum