Thrift: OCaml TSocket fix

Summary: Now closes input channel on close. Also, transport exceptions are cleaner.
Reviewed by: mcslee
Test plan: Yes
Revert plan: yes


git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665198 13f79535-47bb-0310-9956-ffa450edef68

8 files changed, 72 insertions, 56 deletions

diff --git a/lib/ocaml/README b/lib/ocaml/README
index 9f871fbd9..b3043676a 100644
--- a/lib/ocaml/README
+++ b/lib/ocaml/README
@@ -1,18 +1,34 @@
 Library
 -------
-The library abstract classes, exceptions, and general use functions are mostly jammed in Thrift.ml (an exception being TServer). Implementations live in their own files. I'm on the fence about whether it should be done with objects or modules/functors. Right now they are objects. TBinaryProtocol and TSocket are implemented. TServer and TSimpleServer classes are there, but the fastest route to a binary protocol socket server is to use TServer.run_basic_server which uses OCaml's own server abstraction. To that end, there is TChannelTransport which is a transport class parametrized on input and output channels that does nothing but wrap up the input and output functions.
+The library abstract classes, exceptions, and general use functions
+are mostly jammed in Thrift.ml (an exception being
+TServer). 
 
-A note on making the library: Running make should create native and bytecode libraries.
+Generally, classes are used, however they are often put in their own
+module along with other relevant types and functions. The classes
+often called t, exceptions are called E.
+
+Implementations live in their own files. There is TBinaryProtocol,
+TSocket, TThreadedServer, TSimpleServer, and TServerSocket.
+
+A note on making the library: Running make should create native, debug
+code libraries, and a toplevel.
 
 
 Struct format
 -------------
-Structs are turned into classes. The fields are all option types and are initially None. Write is a method, but reading is done by a separate function (since there is no such thing as a static class). I'm still arguing with myself about whether structs should be put in their own modules along with this read function.
+Structs are turned into classes. The fields are all option types and
+are initially None. Write is a method, but reading is done by a
+separate function (since there is no such thing as a static
+class). The class type is t and is in a module with the name of the
+struct.
 
 
-enum format
+enum format 
 -----------
-Enums are put in their own module along with functions to_i and of_i which convert the ocaml types into ints. For example:
+Enums are put in their own module along with
+functions to_i and of_i which convert the ocaml types into ints. For
+example:
 
 enum Numberz
 {
@@ -26,7 +42,7 @@ enum Numberz
 
 ==>
 
-module Numbers =
+module Numberz =
 struct
 type t =
 | ONE
@@ -51,18 +67,29 @@ type userid Int64.t
 
 exception format
 ----------------
-Exceptions are kind of ugly since the exception structs can't be thrown directly. They also have this exception type which has the name BLAHBLAH_exn. For example, for an exception Xception you get:
+The same as structs except that the module also has an exception type
+E of t that is raised/caught.
 
-exception Xception_exn of xception
+For example, with an exception Xception,
+raise (Xception.E (new Xception.t))
+and
+try
+  ...
+with Xception.E e -> ...
 
 list format
 -----------
-Lists are turned into OCaml native lists
+Lists are turned into OCaml native lists.
 
 Map/Set formats
 ---------------
-These are both turned into Hashtbl.t's.
+These are both turned into Hashtbl.t's. Set values are bool.
 
 Services
 --------
-The client is a class "client" parametrized on input and output protocols. The processor is a class parametrized on a handler. A handler is a class inheriting the iface abstract class. Unlike other implementations, client does not implement iface since iface functions must take option arguments so as to deal with the case where a client does not send all the arguments.
+The client is a class "client" parametrized on input and output
+protocols. The processor is a class parametrized on a handler. A
+handler is a class inheriting the iface abstract class. Unlike other
+implementations, client does not implement iface since iface functions
+must take option arguments so as to deal with the case where a client
+does not send all the arguments.
diff --git a/lib/ocaml/src/Makefile b/lib/ocaml/src/Makefile
index 723402b11..20b098620 100644
--- a/lib/ocaml/src/Makefile
+++ b/lib/ocaml/src/Makefile
@@ -2,6 +2,6 @@ SOURCES = Thrift.ml TBinaryProtocol.ml TSocket.ml TChannelTransport.ml TServer.m
 RESULT = thrift
 LIBS = unix threads
 THREADS = yes
-all: native-code-library byte-code-library top
+all: native-code-library debug-code-library top
 OCAMLMAKEFILE = ../OCamlMakefile
 include $(OCAMLMAKEFILE)
diff --git a/lib/ocaml/src/TBinaryProtocol.ml b/lib/ocaml/src/TBinaryProtocol.ml
index 748423f96..fa84e7125 100644
--- a/lib/ocaml/src/TBinaryProtocol.ml
+++ b/lib/ocaml/src/TBinaryProtocol.ml
@@ -114,7 +114,7 @@ object (self)
     let ver = self#readI32 in
       if (ver land version_mask != version_1) then
         (print_int ver;
-        raise (P.TProtocolExn (P.BAD_VERSION, "Missing version identifier")))
+        raise (P.E (P.BAD_VERSION, "Missing version identifier")))
       else
         let s = self#readString in
         let mt = P.message_type_of_i (ver land 0xFF) in
diff --git a/lib/ocaml/src/TChannelTransport.ml b/lib/ocaml/src/TChannelTransport.ml
index 89ae352cd..5407a8e88 100644
--- a/lib/ocaml/src/TChannelTransport.ml
+++ b/lib/ocaml/src/TChannelTransport.ml
@@ -3,14 +3,18 @@ module T = Transport
 
 class t (i,o) =
 object (self)
+  val mutable opened = true
   inherit Transport.t
-  method isOpen = true
+  method isOpen = opened
   method opn = ()
-  method close = ()
+  method close = close_in i; opened <- false
   method read buf off len = 
-    try 
-      really_input i buf off len; len
-    with _ -> T.raise_TTransportExn ("TChannelTransport: Could not read "^(string_of_int len)) T.UNKNOWN
+    if opened then
+      try 
+        really_input i buf off len; len
+      with _ -> raise (T.E (T.UNKNOWN, ("TChannelTransport: Could not read "^(string_of_int len))))
+    else 
+      raise (T.E (T.NOT_OPEN, "TChannelTransport: Channel was closed"))
   method write buf off len = output o buf off len
   method flush = flush o
 end
diff --git a/lib/ocaml/src/TServer.ml b/lib/ocaml/src/TServer.ml
index a4dcc4428..5fb8089bb 100644
--- a/lib/ocaml/src/TServer.ml
+++ b/lib/ocaml/src/TServer.ml
@@ -17,8 +17,7 @@ let run_basic_server proc port =
                            let trans = new TChannelTransport.t (inp,out) in
                            let proto = new TBinaryProtocol.t (trans :> Transport.t) in
                              try
-                               while proc#process proto proto do () done;
-                               ()
+                               while proc#process proto proto do () done; ()
                              with e -> ()) (Unix.ADDR_INET (Unix.inet_addr_of_string "127.0.0.1",port))
 
 
diff --git a/lib/ocaml/src/TServerSocket.ml b/lib/ocaml/src/TServerSocket.ml
index ac98b087b..9e797060d 100644
--- a/lib/ocaml/src/TServerSocket.ml
+++ b/lib/ocaml/src/TServerSocket.ml
@@ -11,11 +11,12 @@ object
       Unix.listen s 256
   method close =
     match sock with
-        Some s -> Unix.shutdown s Unix.SHUTDOWN_ALL; Unix.close s; sock <- None
+        Some s -> Unix.shutdown s Unix.SHUTDOWN_ALL; Unix.close s; 
+          sock <- None
       | _ -> ()
   method acceptImpl =
     match sock with
         Some s -> let (fd,_) = Unix.accept s in
                     new TChannelTransport.t (Unix.in_channel_of_descr fd,Unix.out_channel_of_descr fd)
-      | _ -> Transport.raise_TTransportExn "ServerSocket: Not listening but tried to accept" Transport.NOT_OPEN
+      | _ -> raise (Transport.E (Transport.NOT_OPEN,"TServerSocket: Not listening but tried to accept"))
 end
diff --git a/lib/ocaml/src/TSocket.ml b/lib/ocaml/src/TSocket.ml
index c02f1ebac..c74864a97 100644
--- a/lib/ocaml/src/TSocket.ml
+++ b/lib/ocaml/src/TSocket.ml
@@ -9,23 +9,28 @@ object (self)
   method isOpen = chans != None
   method opn = 
     try
-      chans <- Some(Unix.open_connection (Unix.ADDR_INET ((Unix.inet_addr_of_string host),port)))
+      let addr = (let {Unix.h_addr_list=x} = Unix.gethostbyname host in x.(0)) in
+        chans <- Some(Unix.open_connection (Unix.ADDR_INET (addr,port)))
     with _ -> 
-      T.raise_TTransportExn 
-        ("Could not connect to "^host^":"^(string_of_int port)) 
-        T.NOT_OPEN
-  method close = match chans with None -> () | Some(inc,_) -> (Unix.shutdown_connection inc; chans <- None)
+      raise (T.E (T.NOT_OPEN, ("TSocket: Could not connect to "^host^":"^(string_of_int port))))
+
+  method close = 
+    match chans with 
+        None -> () 
+      | Some(inc,out) -> (Unix.shutdown_connection inc; 
+                          close_in inc;  
+                          chans <- None)
   method read buf off len = match chans with
-      None -> T.raise_TTransportExn "Socket not open" T.NOT_OPEN
+      None -> raise (T.E (T.NOT_OPEN, "TSocket: Socket not open"))
     | Some(i,o) -> 
         try 
           really_input i buf off len; len
-        with _ -> T.raise_TTransportExn ("TSocket: Could not read "^(string_of_int len)^" from "^host^":"^(string_of_int port)) T.UNKNOWN
+        with _ -> raise (T.E (T.UNKNOWN, ("TSocket: Could not read "^(string_of_int len)^" from "^host^":"^(string_of_int port))))
   method write buf off len = match chans with 
-      None -> T.raise_TTransportExn "Socket not open" T.NOT_OPEN
+      None -> raise (T.E (T.NOT_OPEN, "TSocket: Socket not open"))
     | Some(i,o) -> output o buf off len
   method flush = match chans with
-      None -> T.raise_TTransportExn "Socket not open" T.NOT_OPEN
+      None -> raise (T.E (T.NOT_OPEN, "TSocket: Socket not open"))
     | Some(i,o) -> flush o
 end
         
diff --git a/lib/ocaml/src/Thrift.ml b/lib/ocaml/src/Thrift.ml
index 8ff5fa9d5..92c015b60 100644
--- a/lib/ocaml/src/Thrift.ml
+++ b/lib/ocaml/src/Thrift.ml
@@ -9,11 +9,6 @@ object
   method set_message s = message <- s
 end;;
 
-exception TExn of t_exn;;
-
-
-
-
 module Transport =
 struct
   type exn_type = 
@@ -23,19 +18,7 @@ struct
       | TIMED_OUT
       | END_OF_FILE;;
 
-  class exn =
-  object
-    inherit t_exn
-    val mutable typ = UNKNOWN
-      method get_type = typ
-      method set_type t = typ <- t
-  end
-  exception TTransportExn of exn
-  let raise_TTransportExn message typ =
-    let e = new exn in
-      e#set_message message;
-      e#set_type typ;
-      raise (TTransportExn e)
+  exception E of exn_type * string
 
   class virtual t =
   object (self)
@@ -49,10 +32,7 @@ struct
         while !got < len do
           ret := self#read buf (off+(!got)) (len - (!got));
           if !ret <= 0 then
-            let e = new exn in
-              e#set_message "Cannot read. Remote side has closed.";
-              raise (TTransportExn e)
-          else ();
+            raise (E (UNKNOWN, "Cannot read. Remote side has closed."));
           got := !got + !ret
         done;
         !got
@@ -260,7 +240,7 @@ struct
       | SIZE_LIMIT
       | BAD_VERSION
 
-  exception TProtocolExn of exn_type * string;;
+  exception E of exn_type * string;;
            
 end;;   
 
@@ -280,7 +260,7 @@ struct
 end
 
 
-
+(* Ugly *)
 module Application_Exn =
 struct
   type typ=
@@ -336,7 +316,7 @@ struct
   let read (iprot : Protocol.t) =
     let msg = ref "" in
     let typ = ref 0 in
-      iprot#readStructBegin;
+      ignore iprot#readStructBegin;
       (try 
            while true do
              let (name,ft,id) =iprot#readFieldBegin in