EpollSelector broken?

lars_kirchhoff Joined: 05/27/08 Posts: 59

Posted: 03/23/09 15:24:22 Modified: 03/23/09 15:27:40 I grabbed the latest trunk (4435) and used it with a current project I'm working on. In this project I used the EPollSelector.reregister() function. After updating tango, I changed all occurrences of the EPollSelector.reregister() function to EPollSelector.register(), but unfortunately it doesn't work. After digging in a bit I noticed that the SelectionKey? class has been changed to a struct. After changing it back to a class with a constructor, I was able to modify EPollSelector to use a pointer to change the key event if it is found in _keys, just as it was like in the old reregister function. I was now wondering why the SelectionKey? class has been changed to a struct. Here is the patch for EPollSelector: Index: EpollSelector.d =================================================================== --- EpollSelector.d (revision 4435) +++ EpollSelector.d (working copy) @@ -224,17 +224,17 @@ } body { - auto key = conduit.fileHandle() in _keys; + SelectionKey* key = (conduit.fileHandle() in _keys); if (key !is null) { epoll_event event; - key.events = events; - key.attachment = attachment; + (*key).events = events; + (*key).attachment = attachment; event.events = events; - event.data.ptr = cast(void*) key; + event.data.ptr = cast(void*) *key; if (epoll_ctl(_epfd, EPOLL_CTL_MOD, conduit.fileHandle(), &event) != 0) { @@ -244,7 +244,7 @@ else { epoll_event event; - SelectionKey newkey = SelectionKey(conduit, events, attachment); + SelectionKey newkey = new SelectionKey(conduit, events, attachment); event.events = events; Here is a sample program, which i used to test the change. It doesnt work with the latest trunk, but works when SelectionKey? is modified to be a class and the modified EPollSelector as above: module epoll; private import tango.io.selector.EpollSelector; private import tango.io.Stdout; private import tango.net.Socket, tango.net.ServerSocket, tango.net.SocketConduit, tango.net.http.HttpConst; /** * infinite wait for EPoll */ private const int EPOLLWAIT_INFINITE = -1; void main(char[][] args) { SocketConduit _conduit; SocketConduit[int] _connections; EpollSelector _selector; int _event_count; int _conduit_filehandle; ServerSocket socket = new ServerSocket(new InternetAddress(8000),400,true); /** * initialize EpollSelector */ _selector = new EpollSelector(); _selector.open(1, 1); _selector.register(socket, Event.Read); /** * set Server Socket to non blocking */ socket.socket.blocking(false); char[16] buffer; while(true) { try { _event_count = _selector.select(EPOLLWAIT_INFINITE); } catch (Exception e) {} if ( _event_count > 0 ) { foreach (SelectionKey key; _selector.selectedSet()) { try { Stdout.formatln("try to get conduit."); _conduit = (cast(ServerSocket) key.conduit).accept(); _conduit_filehandle = _conduit.fileHandle(); _connections[_conduit_filehandle] = _conduit; Stdout.formatln("got conduit."); } catch (Exception e) { _selector.register(key.conduit, Event.Read); } if (key.isReadable()) { int count = (cast(SocketConduit) _conduit).read(buffer); if (count != IConduit.Eof) { _selector.register(key.conduit, Event.Write, key.attachment); Stdout.formatln("read input."); } else { _connections[_conduit_filehandle].shutdown(); _connections[_conduit_filehandle].detach(); } } if (key.isWritable()) { Stdout.formatln("start output."); _connections[_conduit_filehandle].write("output"); _selector.register(key.conduit, Event.Read, key.attachment); Stdout.formatln("wrote to output."); } if (key.isError() || key.isHangup() || key.isInvalidHandle()) { _selector.unregister(key.conduit); _connections[_conduit_filehandle].shutdown(); _connections[_conduit_filehandle].detach(); } } } } scope(exit) { _selector.close(); } }

Author	Message
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/23/09 19:29:25 -- Modified: 03/23/09 20:52:02 by schveiguy Selection key was changed to a struct to better describe the contract between the selection keys and the selector. In some instances, the actual key used to store the events to be selected was being returned after a selection (with the resulting events modified), which resulted in not being able to iterate over registered keys (a requested enhancement). A selection key is just a pod type that pairs a conduit with event flags and user data. I don't see how making it a class vs. struct would help. Please define "Doesn't work". Your example code doesn't seem to be correct. Can you describe what this code is supposed to be doing?
lars_kirchhoff Joined: 05/27/08 Posts: 59	Posted: 03/24/09 14:55:26 the code should start a socket and wait for requests. if something is written and send to the server socket it should simple output "output" to the SocketConduit? and then detach. In detail it should read the input (in the provided case the input is not used) if a request is retrieved (key.isReadable()) and register the same SelectionKey? for writing. Then it should be entering the write event (key.isWritable()), output "output" and leave. What it does is to retrieve the request and then to jump back to the wait of EPollSelector. The output generated by Stdout commands should look like: try to get conduit. got conduit. read input. start output. wrote to output. But instead the program just outputs: try to get conduit. got conduit. read input. and nothing is written to the SocketConduit?. Further more the socket is not closed and no further requests are possible.
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/24/09 17:29:32 This is not what your code is doing. Here is what your code is doing: Create a server socket Select on the server socket, waiting for incoming connections in the following loop: Accept the incoming connection. Note you always do this, even if the socket was selected for writing or if it's not a server socket, which would result in a Seg Fault. In the *same loop iteration*, if the server socket was readable (which it was, only reason you get in here), read from the new connection (this makes no sense to me), If the new connection has no data, close it (huh?). If it does have data, re-register the *server socket* to write mode (huh?). In the *same loop iteration*, if the server socket was writable, write to the conduit that was last accepted (huh?) and switch the *server socket* back to read mode (huh?). If any errors occurred on the server socket, unregister the server socket, then close the accepted socket. As you can see, there are numerous problems with your code. I'm surprised it ever worked. I think you are forgetting to register your accepted connections with the selector. Also, you have several places that you reregister with the selector. This isn't necessary. You should register connections for every possible event you envision them being selected for, then only reregister in special cases (closed one direction, need to change registered data, etc.). So, let's re-examine how the design *should* look (in pseudocode): while(true) { _event_count = _selector.select(); // infinite is default if(_event_count > 0) { foreach(key; selector.selectedSet) { if(key.conduit is server socket) { accept the client; register the client with the selector as readable; } else // client { if(client is readable) { read data; if(eof) { close the client, unregister it; } else { write answer; // no need to select writable, that is only used if you try to // write data and it doesn't write the whole message } } else if(error occurred) { unregister the client; close the client; } } } } } I'll leave it to you to fill in the details.
larsivi Joined: 03/27/04 Posts: 614	Posted: 03/24/09 17:36:30 Deja Vu!
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/24/09 17:59:12 That is hilarious! I didn't expect to see such a recent date, was it really a month ago that we talked about this?
kris Joined: 03/27/04 Posts: 581	Posted: 03/24/09 18:37:02 But to be fair, asynchronous I/O is inevitably non-trivial. Comprehension of state-machines is one of those things humans are exceedingly poor at :-)
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/24/09 18:54:44 Please understand, I didn't mean it was hilarious that he still hasn't solved the problem, I agree this kind of thing is difficult to get right. I thought it was hilarious that I completely forgot that I already had this exact discussion just about a month ago :) At least I came up with the same analysis :D
kris Joined: 03/27/04 Posts: 581	Posted: 03/24/09 19:59:37 yarrr :)
lars_kirchhoff Joined: 05/27/08 Posts: 59	Posted: 03/25/09 14:59:41 -- Modified: 03/25/09 15:05:45 by lars_kirchhoff -- Modified 3 Times I knew that we would refer to the old discussion ;).. but anyway in the old discussion some questions are still open and in the new thread some new ones are added, which have not been answered, yet. One question I still have regarding your code is how to test whether key.conduit is ServerSocket? or SocketConduit?? As far as I can see key.conduit is of type tango.io.model.IConduit.ISelectable, when returned from: foreach(key; selector.selectedSet) So how do I test it? Second, refering to the change of SelectionKey? from class to struct how do you explain the different Stdout outputs from the sample code? The output were generated on the console from where the program was started, while I run a "telnet localhost 8000". /L
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/25/09 19:33:54 You can either test by casting (if you have more than one server socket) or comparing the reference: if(serverSocket = cast(ServerSocket)key.conduit) //or if(key.conduit is socket) The version with structs is what I expected to happen (receive an incoming connection, read the data, then reregister the server socket as writable, never is selected again). I can't explain the version with classes because I'm not sure what you did or what you compiled against. Your code is invalid, so it's difficult to say why it may appear to work (I'm sure it's not working like you want it to). I'm 100% sure the difference between SelectionKey being a class or struct is not causing any problems.
lars_kirchhoff Joined: 05/27/08 Posts: 59	Posted: 03/30/09 13:47:34 -- Modified: 03/30/09 13:47:48 by lars_kirchhoff Thanks for the help. I've implemented it as you suggested and it works ok. One last question. How do I check if the key.conduit is a SocketConduit?? I tried: if(key.conduit is SocketConduit) but this didn't work. /L
larsivi Joined: 03/27/04 Posts: 614	Posted: 03/30/09 17:04:12 Look to the other if that schveiguy created, the one with the cast.
schveiguy Joined: 08/02/07 Posts: 128	Posted: 03/30/09 18:44:11 Yes, as lars said, do if(auto clientSocket = cast(SocketConduit)key.conduit)) To help you understand, it is helpful to know what the compiler is doing here. When you write x is y, the compiler is comparing the exact bits of x and y to see if they are identical (in the case of reference types such as interfaces and classes, the compiler checks if they point to the same exact instance). I used it to check if the conduit was *the same instance* as the object you were looking for in my prior example, i.e. the server socket. However, you have multiple clients, so to use 'is', it would look something like this: if(key.conduit is client1Socket || key.conduit is client2Socket || ...) Which is impractical and hard to code (you'd need a for loop because you have unknown number of clients). Instead, what you want is to check if the instance is a certain type. I know other languages (such as C#) use 'is' to denote 'is a', but D uses casts. Casting from one class type to another is not a forced change of type. It only works if the 'is a' relationship is correct. Otherwise, it returns null. The compiler does this by looking at the object's ClassInfo to see if it can be cast properly. Note that my abbreviated form both checks to see if the conduit is a SocketConduit, AND creates a local variable 'clientSocket' to be able to work with the SocketConduit members. This is a pretty handy construct in D, I'm not sure if any other languages have something so cool :) In fact, you may want to use this to check for the server socket also, since you have to cast anyways. One other thing, were all your questions resolved from the other thread as well? You mentioned before there were still some unanswered questions. Thanks -Steve
lars_kirchhoff Joined: 05/27/08 Posts: 59	Posted: 06/25/09 18:13:14 sorry for the late answer. I didn't see your last post. And thanks for the comprehensive answer. It works now quite well. /lars