Client development

[ZsigE]

We need a client! In our meeting on September 2nd, I agreed to take on responsibility for delivering a working prototype of a client (probably not feature-complete or polished, but doing SOMETHING useful!) with a first draft of a curses-based CLI, by September 16th. This issue tracks that.

[ZsigE]

Added some basic code for the CLI in this commit.

[ZsigE]

Right, the prototype CLI with some very basic functionality (join/leave CLI, receive messages) is now committed, in very much first-draft form, in this commit. It won't currently do a lot because I haven't written any client code to make it go, and I haven't actually tested it yet (do you like my cowboy hat?), but it is definitely a start.

[ZsigE]

Decent chunk of code added today. This still isn't finished (and although I've added some interactive UT for the CLI, I haven't yet got it working properly), but it should give you enough of an idea of the overall architecture that you can start to think about how you might want to extend it.

[ZsigE]

CLI UT is now working, Django and Tornado proxy code is first-pass complete, and I've started on tying it all together in the core module.

[peterbrittain]

Nice! I meant to post this earlier, but clearly forget to press the comment button...

I cheekily fixed up the curses code in the client to try to get a basic mainline. I believe that you can now get to the credits as a result.

Looking forwards, I have a couple of thoughts:

1. The split of client and cli wasn't clear to me when I looked at the code - especially with the UT basically starting everything up. Maybe we should move the UT to a separate file?
2. As I'm sure Phil would point out and is no doubt already planning, we need to beef up the current CLI interface.
   • There is no help yet.
   • There is no interactive prompt - e.g. for a message to send - which is going to need more than just single characters.
   • The exit and credits are hard-coded into the input.

The latter item got me thinking... I was wondering if we should create a sort of pseudo-shell to address this. We could use the current menu structure to make a context sensitive list of commands. Each command is then typed into the input window and parsed as "(command) (options)". This invokes the same basic functions as now, just passing in the parameters (if any).

Finally, we add a global context for the help, exit and credits commands, using the same basic menu structure to define these commands.

[ZsigE]

I noticed you'd tweaked it a bit - thanks! With regards to your thoughts:

1. The UT is just for the client code and takes the place of what the client proper will do - that code is still to do, I may try to get that done tomorrow if I have time. The idea is that the client will create and start the CLI by importing the cli module and instantiating the Cli class, then running its run() method in a separate multiprocessing Process.
2. It is a touch rudimentary right now - just wanted to get something working. My current thoughts for your separate points:
   • Help could probably be implemented by having "?" or "h" be a globally reachable command that pops up a new window over the existing ones with context-sensitive help.
   • Interactive prompts could be handled by having a command for "send message" which creates a window over the input window for text entry, then removes it and sends the entered text. That would probably require adding a callback class variable to MenuItem so that a menu item can run arbitrary code when selected.
   • Exit is actually a special option that appears in every menu level - for every level other than the top one it goes back to that menu level's parent, and for the top level it exits. I'd rather have that added automatically than have to specify it on every menu level. Credits could be added to the start menu easily enough, or we could replace the "A" trigger with something less likely to be used for an actual menu item ("~", maybe).

I'd be wary of implementing a shell, personally. Seems a little over the top for what we want to do (also I quite like the snappy response by having single-key inputs). Open to being persuaded though.

[peterbrittain]

The level of shell logic I was proposing is less involved than this as it is simple text parsing (split on first space) and just requires the one path (scrolling window line input) to enter any data, which you are going to need for your new pop-up window. Also, the global context I proposed above just moves all your hard-coded functions and moves them into a parallel menu item - thus making the code more consistent.

Those aren't the real issues, though (but I'm happy to take offline and discuss directly if you'd like to explore it some more).

The real issue is what type of UI is best. Maybe one for Neil to comment on (after getting some ideas from Brian Lockwood)?

[peterbrittain]

In the meantime, I've added just enough code to client.py to allow it to talk to cli.py and thus connect to a real server. The client can now register/unregister users and create/join/delete a conga. Still loads of hard-coded values right now, though, coz there is no interactive prompt (e.g. for conga name, password, etc) yet.

[ZsigE]

Client-related actions arising from today's meeting:

• Client needs some form of arbitrary text input to send messages - permission to send messages is restricted on the server side, the client should generally be able to send them
• Tornado communication does not currently work (top priority!). Dependency on @Lukasa to ensure that the server side works.
• Client does not currently handle loss of communication (expected or unexpected) with the Tornado server

I own all of these right now, with the dependencies as noted.

[Lukasa]

Ok @peterbrittain, the newest release of the Tornado server has some Postgres code added. I want to stress that right now it's completely untested, and I don't have time tonight to get a test setup ready. Nevertheless, to use it against the production postgres db you should run the Tornado server like so: python tornado_server/tornado_main.py --pgname=piconga --pguser=piconga --pgpass=piconga

Let me know if you encounter difficulties.

[peterbrittain]

Over to you @ZsigE... I've just updated the client to point at the EC2 server and started the tornado server on that VM.

[ZsigE]

I've had a go at this tonight, but haven't got very far yet. Managed to get Tornado installed onto my dev VM (with some fun along the way of having to switch to Python 2.7), but I haven't yet got a postgres DB running locally so can't get the Tornado server running. May have to try debugging the client by using the real server for now, which hopefully shouldn't cause too many problems.

[Lukasa]

Tornado will happily run against the sqlite db for testing purposes. =)

[peterbrittain]

Indeed! Or you could run against the live EC2 server (that will be used by default when you start up the latest client code).

[ZsigE]

Well, using sqlite makes things easier...still haven't sorted out the bugs, but I have made things a bit easier to debug by putting some basic ping functionality into the client so that we now actually try to use the Tornado server. Also added username and password entry before startup.

[ZsigE]

Getting closer...have also added proper logging and stderr redirection so that exceptions don't get splatted all over the CLI.

[ZsigE]

Argh. Just worked out what's going on. It's not pretty.

Basically, I massively misunderstood how Python's multiprocessing module works. What I was trying to do was create a CLI instance in the client, farm it out into a separate process to loop around and not get in the way of anything else, and then call its methods from the client code.

This doesn't work, because what actually happens when you create the child process with multiprocessing is that the child clones the CLI instance from the core client and then runs it in a completely separate way - so changes to the original instance don't affect the one in the child process and vice versa. Similarly, methods called on the CLI instance from the core client will run on the core instance, and therefore won't see any events that the CLI puts on its queues.

@peterbrittain solved this for the CLI case by changing the interface so the core would only ever talk to the queues exposed by the CLI. The bugs we're seeing when trying to talk to the Tornado server are caused because we are trying to send a message to the instance of the Tornado send/receive proxy living in the core code, which hasn't set up a socket to talk to the Tornado server because that's been done by the instance in the child process.

Clear as mud? Good. Not immediately sure how to fix this - I think I'll have to change the API a bit so that we only talk over queues to the Tornado S/R as well. I'l keep working on it, though.

[peterbrittain]

@ZsigE Yup - that's exactly the problem. I was working on it this evening and have hacked in the same solution for tornado. Resulting code is now not very pretty. Sorry. Might be better to move all the socket comms to the mainline process and so just have 2 "threads" rather than the 3 we have right now?

[ZsigE]

Right, made some improvements! The external API to the Tornado send/receive proc is now explicitly outside the send/receive class and therefore doesn't run the risk of turning up in a duplicate instance. Should still be conceptually separate and fairly clean though. And we can now send messages to the Tornado server! Haven't yet got them to a second client though. The buglist that I know of now looks like this.

• When we send a message to the Tornado server, it spams the client's socket with zero-length messages apparently endlessly. Any idea why, @Lukasa?
• (While you're about it, can the dev Tornado server print out what it's doing like the Django server does? Not sure where to start with that.)
• Leaving the Conga closes the connection to the Tornado server and it never reopens. Need to cope with closing its connection and automatically restarting it. Fixed!
• We still need to cope with the connection being closed from the server side. Toying with the possibility of a third queue from the Tornado S/R into the client core that carries management messages ("the server closed the connection","everything is going wrong" etc.) Partly fixed - repurposed the two existing queues to carry metadata too.
• Pressing a button to, for instance, join a Conga immediately moves us into the In-Conga menu regardless of whether that worked. We need to allow menu movement based on events.
• Pings are a useful testing tool, but we will need arbitrary text input too.

[Lukasa]

@ZsigE I'll investigate both of those to see what I can see.

[ZsigE]

In the meantime, I've just crossed off one of those issues. Have also spotted one other thing while testing what happens if the server closes the connection - I don't think the Tornado server actually was spamming us, as we still loop madly reporting zero-length messages even after killing the server entirely! Either it's something weird to do with the localhost connection I'm testing with, or my VM's networking code is playing up, or (most likely) I don't understand the socket connection properly. Will investigate...

EDIT: Wait, scratch that - I was pointing at the real server, not the VM. Looks like the server dies horribly when you try to talk to it (reference before assignment, participant.py:158). Back to Cory to look at that!

[peterbrittain]

OK - I've had some time to look at these issues tonight:

• I think the zero length message was just a mis-understanding of the sockets API. It is allowed to return no data. Sample code out there tends to check for this, so we now just bail in the client if there is no message.
• I fixed various logging bugs in the tornado server - including the assignment error @ZsigE mentions.
• I also sorted various disconnects in the client threads - where the data structures didn't match up.

Net result: I GOT ONE CLIENT TO PING ANOTHER!!

The whole system is still pretty flaky, though. I need to restart the clients and the tornado server to re-establish comms on a regular basis. I suspect that's coz they're not tidying up properly yet - e.g. there seems to be some persistence of state on the tornado server after a conga is deleted and there are connection problems once the server drops the link on the client.

[Lukasa]

@peterbrittain Excellent work! I suspect the code currently doesn't support removing Congas when the last participant is removed, which is presumably the problem. I'll try to take a look sometime soon.

[peterbrittain]

@Lukasa Thank you. It could also be that I've not been rigorous in taking down the conga and registrations...

[ZsigE]

Finally had the time and energy to do a bit more on this. We now move between CLI menu levels only when the server signals that we can (and in the meantime we flag to the user that we are waiting); I have also added a somewhat rudimentary form of freeform text input for sending messages.

Note that this doesn't do exactly what I wanted it to - you only see the messages you sent (or that other people sent) once you leave freeform text mode. That does make it kind of tricky to use this properly as a chat program, but it feels like we should be able to sort that out (maybe with a bit of clever multithreading).

[ZsigE]

Fixed freeform text, added the ability to choose the conga you want to join/create. The tradeoff for getting the event window to keep scrolling was that the cursor has to remain hidden (otherwise it gets stolen by the event window), but I think we can cope with that.

[ZsigE]

Updated to match @Lukasa's changes, also tweaked the formatting of the printed messages so we can see who's speaking. I am still seeing error messages in the localhost Tornado server when the last user of the conga leaves: [E 131207 21:05:10 conga:145] Attempted to remove participant 5 from incorrect conga 2. [E 131207 21:05:10 participant:186] Failed to remove 5 from conga 2 because of Not in conga.

Is that something I'm doing wrong? Can't tell immediately.

[Lukasa]

Hmm, I can't reproduce that. What're the repro steps? Is it consistent?

That particular error usually comes up if we've removed the participant from the list of participants in a conga. Which is unexpected.

[ZsigE]

Hmm. I assumed it was leftover state in the DB from earlier attempts, so I wiped it and restored from GitHub. Now I can't get more than one participant into a conga at all (using either the local server or the main one). That's annoying. I'll do a little more experimenting, but I'm not sure what's going on here.

[peterbrittain]

I just had a go with the latest code and a development server this morning and it all looks good to me! I managed to create, join and leave congas as many times as I like with the 2 clients I was testing with.

[ZsigE]

Weird. Well, I'm not complaining if that all looks good! I'll have some time tonight to fix stuff up if you spot anything late-breaking that I need to deal with.

[peterbrittain]

Yeah - it all looks like it's working to me for 2 clients... Sadly, 3 clients aren't working so well. The message gets from the sender to the first person in the conga, but not the next. IIRC, this is deliberate... It's supposed to be a conga after all! I think the fix is for the client to re-send the received message. Is that right, @Lukasa ? If so, what does the client need to do to indicate the end of the message chain?

[Lukasa]

@peterbrittain The specific problem is that the conga has no way to terminate a message, so if that change is made we'll loop the message for all time (see #15). That's pretty bad. I'll whack in something this weekend so that we can remove the patch on the client.

[peterbrittain]

@Lukasa Brian tells me that the conference is tomorrow! We really need to resolve it tonight if that's at all possible... Maybe we can hack something in the client tonight to stop sending the message when it sees it is from itself? What do you think @ZsigE?

[Lukasa]

Eep, that's soon! I can see what I can do this evening.

[Lukasa]

OK, I took a look and whacked in a new header: Message-ID. This is assigned by the tornado server whenever it receives a MSG message without such a header on it.

@ZsigE The client should be changed to pass the Message-ID header through on any message that isn't being originally sent from it. If the client drops the Message-ID header the server will treat it as a new message.

Please note that this is pretty stupid: there's a really easy DoS attack you can perform by simply spamming new messages until the server runs out of memory to track them. I don't think I can do much better in the time I have available. I'll check my changes in shortly.

[Lukasa]

Well, that diff makes me very sad, but it's the best I can do at short notice on a tight budget. =) Seems to work OK though.

[ZsigE]

Added message passing from the client, but I'm having a horrible time trying to test it (still can't get two clients into the same conga). Can someone give my latest checkins a try?

[Lukasa]

Just done it. Seems to work fine, though the indentation level of the message increases by one space for each extra participant it goes through, which is weird. =D

[ZsigE]

Bet I know why. Give me a second...

[ZsigE]

...OK, try that again. I think I was extending the value of the From header by a space every time because I hadn't taken the space after the colon into account in "From: username".

[Lukasa]

Yep, fixed. Looks good to me! =D

[peterbrittain]

And I've just updated the live EC2 server to use the new server code. Reckon we're clear to go for the big day. Well done, guys!