Proposed patch for threadsafe access to octave internal data

[John Swensen]

I have attached my first shot at a patch to allow the IDE I am writing 
threadsafe access to octave internal data.  The basic idea is that I 
have created a class called octave_server that has a mutex protected set 
of data and acts as an intermediary between my IDE and octave.  I have 
registered a function with the octave_rl_event_hook so that when 
readline is idle, a function is called that processes data sent from the 
IDE to octave (e.g. requests for variables values, breakpoint 
operations, etc) and then fills in buffers for data that is updated 
regularly (e.g. the names and properties of variables that are currently 
in scope, new lines of history information, etc.) for subsequent 
asynchronous access by the IDE.  According to the readline 
documentation, the event hook is called at most 10 times per second.  
This is more than adequate for the needs of the IDE.

I have done some preliminary timing tests on my MacbookPro and the 
entire event hook function takes approximately 0.3 milliseconds.  Take 
note that this only occurs while octave is sitting idle at the readline 
prompt.  Otherwise, the event hook function is not called, so this extra 
work *is not* being done during octave computations.

Since I am kindof new to this whole patch submitting for octave process, 
I am open to suggestions.  I tried to think through the design of this 
"gateway" class, but there is a good possibility I make horrible 
assumptions and clear (to everyone but me) flaws.  If I can get this (or 
something similar to this) accepted, it will bring me one step closer to 
having a much more stable version of my IDE, and provide for a path 
forward for including an integrated debugger and other eye candy like 
mouse-over variable inspections, etc.  FYI, this IDE (Sourceforge 
octavede project) is based on the VTE method described by John Eaton in 
several mailing list threads.

John Swensen

P.S. This only adds one line of code to the existing code, where I 
register the readline event hook function in toplev.cc
P.P.S. I'm sure I violated a bunch of your coding standards, so while I 
wait to hear back, I will go look at the official standard and start 
making corrections.
Index: src/Makefile.in
===================================================================
RCS file: /cvs/octave/src/Makefile.in,v
retrieving revision 1.440
diff -u -b -B -r1.440 Makefile.in
--- src/Makefile.in     12 Jun 2007 21:39:27 -0000      1.440
+++ src/Makefile.in     15 Jul 2007 02:18:37 -0000
@@ -107,7 +107,7 @@
        oct-errno.h oct-fstrm.h oct-hist.h oct-iostrm.h oct-map.h oct-obj.h \
        oct-prcstrm.h oct-procbuf.h oct-stdstrm.h oct-stream.h zfstream.h \
        oct-strstrm.h oct-lvalue.h oct.h octave.h ops.h pager.h \
-       parse.h pr-output.h procstream.h sighandlers.h siglist.h \
+       parse.h pr-output.h procstream.h server.h sighandlers.h siglist.h \
        sparse-xdiv.h sparse-xpow.h symtab.h sysdep.h \
        token.h toplev.h unwind-prot.h utils.h variables.h version.h \
        xdiv.h xpow.h \
@@ -177,7 +177,7 @@
        mex.cc oct-fstrm.cc oct-hist.cc oct-iostrm.cc oct-map.cc \
        oct-obj.cc oct-prcstrm.cc oct-procbuf.cc oct-stream.cc \
        octave.cc zfstream.cc oct-strstrm.cc oct-lvalue.cc pager.cc \
-       parse.y pr-output.cc procstream.cc sighandlers.cc \
+       parse.y pr-output.cc procstream.cc server.cc sighandlers.cc \
        siglist.c sparse-xdiv.cc sparse-xpow.cc strfns.cc \
        symtab.cc syscalls.cc sysdep.cc token.cc toplev.cc \
        unwind-prot.cc utils.cc variables.cc xdiv.cc xpow.cc \
Index: src/server.cc
===================================================================
RCS file: src/server.cc
diff -N src/server.cc
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ src/server.cc       15 Jul 2007 02:18:37 -0000
@@ -0,0 +1,359 @@
+/*
+
+Copyright (C) 2007 John P. Swensen
+
+This file is part of Octave.
+
+Octave is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 2, or (at your option) any
+later version.
+
+Octave is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with Octave; see the file COPYING.  If not, write to the Free
+Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+02110-1301, USA.
+
+*/
+
+// Born July 13, 2007.
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <cassert>
+#include <cstdlib>
+#include <cstring>
+#include <ctime>
+
+#include <fstream>
+#include <iostream>
+
+#ifdef HAVE_UNISTD_H
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#include <unistd.h>
+#endif
+
+#include "cmd-edit.h"
+#include "f77-fcn.h"
+#include "file-ops.h"
+#include "file-stat.h"
+#include "lo-error.h"
+#include "oct-env.h"
+#include "pathsearch.h"
+#include "str-vec.h"
+
+#include <defaults.h>
+#include "Cell.h"
+#include "defun.h"
+#include "error.h"
+#include "file-io.h"
+#include "input.h"
+#include "lex.h"
+#include "load-path.h"
+#include "octave.h"
+#include "oct-hist.h"
+#include "oct-map.h"
+#include "oct-obj.h"
+#include "ops.h"
+#include "ov.h"
+#include "toplev.h"
+#include "parse.h"
+#include "procstream.h"
+#include "prog-args.h"
+#include "server.h"
+#include "sighandlers.h"
+#include "sysdep.h"
+#include "ov.h"
+#include "unwind-prot.h"
+#include "utils.h"
+#include "variables.h"
+#include <version.h>
+
+
+octave_server oct_octave_server;
+
+
+/*************************************************************************
+ *
+ *
+ ************************************************************************/
+int server_rl_event_hook_function(void)
+{
+  // TODO: No need to run too quickly.  The documentation says it will run
+  // at most 10 times per second.  This may be too fast and we will need to
+  // artificially slow it down somehow.  Not sure at this time how.
+  oct_octave_server.process_octave_server_data();
+  
+  return 0;
+}
+
+octave_server::octave_server()
+{
+  pthread_mutex_init(&server_mutex,NULL);
+  pthread_mutex_init(&octave_lock_mutex,NULL);
+
+  prevHistLen = 0;
+}
+
+octave_server::~octave_server()
+{
+
+}
+
+/*******************************************************************************
+ 
*******************************************************************************
+ * CLIENT SIDE FUNCTIONS
+ 
*******************************************************************************
+ 
*******************************************************************************/
+std::vector<variable_info_t> octave_server::get_variable_info_list(void)
+{
+  // Acquire the mutex
+  if( pthread_mutex_trylock( &server_mutex ) != 0 )
+    return std::vector<variable_info_t>();
+
+  std::vector<variable_info_t> retval( variable_symtab_list.size() );
+  std::copy( variable_symtab_list.begin(), variable_symtab_list.end(), 
retval.begin() );
+  variable_symtab_list = std::vector<variable_info_t>();
+
+  // Release the mutex
+  pthread_mutex_unlock( &server_mutex );
+
+  return retval;
+}
+
+std::vector<requested_variable_t> octave_server::get_requested_variables(void)
+{
+  // Acquire the mutex
+  if( pthread_mutex_trylock( &server_mutex ) != 0 )
+    return std::vector<requested_variable_t>();
+  std::vector<requested_variable_t> retval( requested_variables.size() );
+  std::copy( requested_variables.begin(), requested_variables.end(), 
retval.begin() );
+  requested_variables = std::vector<requested_variable_t>();
+  // Release the mutex
+  pthread_mutex_unlock( &server_mutex );
+
+  return retval;
+}
+
+status_t octave_server::set_requested_variables_names( 
std::vector<std::string> variables_names )
+{
+  // Acquire the mutex
+  if( pthread_mutex_trylock( &server_mutex ) != 0 )
+    return -1;
+
+  variables_request_list = std::vector<std::string>( variables_names.size() );
+  std::copy( variables_names.begin(), variables_names.end(), 
variables_request_list.begin() );
+  // Release the mutex
+  pthread_mutex_unlock( &server_mutex );
+
+  return 0;
+}
+
+string_vector octave_server::get_history_list(void)
+{
+  // Acquire mutex
+  if( pthread_mutex_trylock( &server_mutex ) != 0 )
+    return string_vector();
+
+  string_vector retval( history_list );
+  history_list = string_vector();
+
+  // Release mutex
+  pthread_mutex_unlock( &server_mutex );
+
+  return retval;
+}
+
+
+/*******************************************************************************
+ 
*******************************************************************************
+ * SERVER SIDE FUNCTIONS
+ 
*******************************************************************************
+ 
*******************************************************************************/
+
+status_t octave_server::process_octave_server_data(void)
+{
+  struct timeval start, stop;
+  gettimeofday(&start, NULL);
+
+  // Acquire mutex
+  if( pthread_mutex_lock( &server_mutex ) != 0 )
+  {
+    octave_stdout << "Error acquiring the octave_server data lock mutex" << 
std::endl;
+    return -1;
+  }
+  
+  process_requested_variables();
+  set_variable_info_list();
+
+  set_history_list();
+
+  // Release mutex
+  pthread_mutex_unlock( &server_mutex );
+
+  gettimeofday(&stop, NULL);
+  double elapsed = stop.tv_sec - start.tv_sec + 1E-6 * (stop.tv_usec - 
start.tv_usec);
+  //octave_stdout << "SERVER ELAPSED: " << elapsed << std::endl;
+
+  return 0;
+}
+
+status_t octave_server::set_variable_info_list( void )
+{
+  static std::vector<variable_info_t> lastVars;
+  std::vector<variable_info_t> currVars;
+
+  //// Borrowed from the 'whos' function
+  // This method prints information for sets of symbols, but only one
+  // set at a time (like, for instance: all variables, or all
+  // built-in-functions).
+
+  // This method invokes print_symbol_info_line to print info on every
+  // symbol.
+
+  //int status = 0;
+
+  // XXX FIXME XXX Should separate argv to lists with and without dots.
+  if ( top_level_sym_tab != NULL )
+  {
+    if ( global_command == 0 )
+    {
+      Array<symbol_record *> xsymbols = top_level_sym_tab->symbol_list ( 
string_vector(), 0xFF, SYMTAB_ALL_SCOPES);
+      Array<symbol_record *> xsubsymbols = top_level_sym_tab->subsymbol_list ( 
string_vector(), 0xFF, SYMTAB_ALL_SCOPES);
+
+      int sym_len = xsymbols.length (), subsym_len = xsubsymbols.length (),
+                                                     len = sym_len + 
subsym_len;
+
+      Array<symbol_record *> symbols (len);
+
+      if (len > 0)
+      {
+        //size_t bytes = 0;
+        //size_t elements = 0;
+
+        int i;
+
+        // Joining symbolic tables.
+        for (i = 0; i < sym_len; i++)
+          symbols(i) = xsymbols(i);
+
+        for (i = 0; i < subsym_len; i++)
+          symbols(i+sym_len) = xsubsymbols(i);
+      }
+
+      for (int j = 0; j < len; j++)
+      {
+        if ( symbols(j)->is_user_variable() )
+        {
+          variable_info_t tempVar;
+          tempVar.variable_name = symbols(j)->name();
+          tempVar.size.push_back( symbols(j)->rows() );
+          tempVar.size.push_back( symbols(j)->columns() );
+          tempVar.byte_size = symbols(j)->byte_size();
+          tempVar.type_name = symbols(j)->type_name();
+
+          currVars.push_back(tempVar);
+        }
+      }
+
+    }
+  }
+  else
+  {
+    std::cout << "Empty top_level_sym_tab: waiting until next iteration" << 
std::endl;
+  }
+
+  if ( lastVars != currVars )
+  {
+    lastVars = currVars;
+    
+    // Copy currVars into octave_server::variable_symtab_list
+    variable_symtab_list = std::vector<variable_info_t>( currVars.size() );
+
+    std::copy( currVars.begin(), currVars.end(), variable_symtab_list.begin() 
);
+  }
+
+  return 0;
+}
+
+status_t octave_server::process_requested_variables( void )
+{
+  // Clear the list of existing requested variables
+  requested_variables = std::vector<requested_variable_t>();
+  
+  //// Get the list of variables and copy them into requested_variables vector
+  // XXX FIXME XXX Should separate argv to lists with and without dots.
+  if ( top_level_sym_tab != NULL )
+  {
+    if ( global_command == 0 )
+    {
+      Array<symbol_record *> xsymbols = top_level_sym_tab->symbol_list ( 
string_vector(), 0xFF, SYMTAB_ALL_SCOPES);
+      Array<symbol_record *> xsubsymbols = top_level_sym_tab->subsymbol_list ( 
string_vector(), 0xFF, SYMTAB_ALL_SCOPES);
+
+      int sym_len = xsymbols.length (), subsym_len = xsubsymbols.length (),
+                                                     len = sym_len + 
subsym_len;
+
+      Array<symbol_record *> symbols (len);
+
+      if (len > 0)
+      {
+        //size_t bytes = 0;
+        //size_t elements = 0;
+
+        int i;
+
+        // Joining symbolic tables.
+        for (i = 0; i < sym_len; i++)
+          symbols(i) = xsymbols(i);
+
+        for (i = 0; i < subsym_len; i++)
+          symbols(i+sym_len) = xsubsymbols(i);
+      }
+
+      for (int j = 0; j < len; j++)
+      {
+        if ( symbols(j)->is_user_variable() && 
+            std::find( variables_request_list.begin(), 
variables_request_list.end(), 
+                       symbols(j)->name() ) != variables_request_list.end() )
+        {
+          requested_variable_t tempVar;
+          tempVar.name = symbols(j)->name();
+          tempVar.ov = symbols(j);
+          requested_variables.push_back(tempVar);
+        }
+      }
+
+    }
+  }
+
+  variables_request_list = std::vector<std::string>();
+
+  return 0;
+}
+
+
+status_t octave_server::set_history_list( void )
+{
+  
+
+  // Build up the current list
+  int currentLen = command_history::length();
+  if ( currentLen != prevHistLen )
+  {
+    for( int i = prevHistLen ; i < currentLen ; i++ )
+      history_list.append( command_history::get_entry(i) );
+    prevHistLen = currentLen;
+  }
+
+  return 0;
+}
+
Index: src/server.h
===================================================================
RCS file: src/server.h
diff -N src/server.h
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ src/server.h        15 Jul 2007 02:18:37 -0000
@@ -0,0 +1,252 @@
+/*
+ *
+ * Copyright (C) 2007 John P. Swensen
+ *
+ * This file is proposed as a part of Octave.
+ *
+ * Octave is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2, or (at your option) any
+ * later version.
+ *
+ * Octave is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Octave; see the file COPYING.  If not, write to the Free
+ * Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ *
+ * */
+
+#if !defined (octave_server_h)
+#define octave_server_h 1
+
+#include <cstdio>
+#include <string>
+#include <vector>
+#include <pthread.h>
+#include "ov.h"
+
+class octave_value;
+class octave_value_list;
+
+
+typedef int status_t;
+
+/**
+ * Enumeration used to identify breakpoint actions
+ */
+typedef enum bp_action_enum
+{
+  BP_ACTION_NONE       = 0,
+  BP_ACTION_STEP_INTO  = 1,
+  BP_ACTION_STEP_OVER  = 2,
+  BP_ACTION_CONTINUE   = 3,
+  BP_ACTION_BREAK      = 4,
+} bp_action_t;
+
+/**
+ * Structure used to store breakpoint info.
+ *
+ * Notes: used for add, remove, list operations, as well as for the 
BreakpointReached event.
+ */
+typedef struct bp_info_struct
+{
+  /**
+   * The full path and filename where the breakpoint resides.
+   */
+  std::string filename;   
+
+  /**
+   * The line number where the breakpoint resides.
+   * In the future, -1 can indicate an existing but disabled breakpoint.  This
+   * assumes that no one will ever have an M file longer than 2Million lines.
+   */
+  int line_number;
+} bp_info_t;
+
+/**
+ * Structure used to store variable information similar to that returned by
+ * the 'whos' function.
+ */
+typedef struct variable_info_struct variable_info_t;
+typedef struct variable_info_struct
+{
+  /**
+   * The name of the variable
+   */
+  std::string variable_name;
+
+  /**
+   * The dimensional size of the variable.
+   */
+  std::vector<int> size;
+
+  /**
+   * The size of the variable in bytes.
+   */
+  unsigned long long byte_size;
+
+  /**
+   * The name of the variable type.
+   */
+  std::string type_name;
+  
+  friend int operator==(const variable_info_t& left,
+                        const variable_info_t& right)
+  {
+    return (left.variable_name==right.variable_name) &&
+         (left.size==right.size) &&
+         (left.byte_size==right.byte_size) &&
+         (left.type_name==right.type_name);
+  }
+  
+} variable_info_t;
+
+typedef struct requested_variable_struct
+{
+  std::string name;
+  octave_value ov;
+} requested_variable_t;
+
+class octave_server
+{
+  private:
+    /**
+     * Mutex variable used to protect access to internal class data.
+     */
+    pthread_mutex_t server_mutex;
+    
+    
+    /**
+     * Mutex variable used to protect access to octave internals on 
asynchronous requests.
+     * 
+     * Notes: This is necessary for asynchronous requests like detailed 
variable information
+     * in a debugger mouse-over, inspection of matrix variables by 
double-clicking in the 
+     * main window, etc.
+     */
+    pthread_mutex_t octave_lock_mutex;
+         
+         
+    /***********************************************************************
+     * DEBUGGING RELATED VARIABLE
+     **********************************************************************/
+    std::vector<bp_info_t> current_breakpoints;
+    std::vector<bp_info_t> breakpoint_reached;
+    std::vector<bp_info_t> added_breakpoints;
+    std::vector<bp_info_t> removed_breakpoint;
+    std::vector<bp_info_t> modify_breakpoints_old;
+    std::vector<bp_info_t> modify_breakpoints_new;
+    bp_action_t           bp_action;
+   
+    /***********************************************************************
+     * VARIABLE INTERROGATION RELATED VARIABLES
+     **********************************************************************/
+    std::vector<variable_info_t> variable_symtab_list;
+    std::vector<std::string>     variables_request_list;
+
+    // NOTE: Create an overloaded operator<< for octave_value to do the
+    // flattening.  This will allow us to append easily to an ostringstream
+    // for output.
+    std::vector<requested_variable_t>    requested_variables;    
+    
+    /***********************************************************************
+     * HISTORY RELATED VARIABLES
+     **********************************************************************/
+    int                         prevHistLen;
+    string_vector                history_list;
+    
+  public:
+
+    octave_server();
+    ~octave_server();
+
+
+    /*************************************************************************
+     *************************************************************************
+     * FUNCTIONS USED TO ACCESS DATA FROM THE CLIENT SIDE
+     *************************************************************************
+     *************************************************************************/
+
+    /***********************************************************************
+     * DEBUGGING RELATED FUNCTIONS
+     **********************************************************************/ 
+    std::vector<bp_info_t> get_breakpoint_list();
+    bool                   is_breakpoint_reached();
+    std::vector<bp_info_t> get_breakpoint_reached();    
+    status_t              add_breakpoint( bp_info_t bp_info );
+    status_t              remove_breakpoint( bp_info_t bp_info );
+    status_t              modify_breakpoint( bp_info_t old_bp_info, bp_info_t 
new_bp_info );
+    status_t              set_breakpoint_action( bp_action_t action );
+   
+    /***********************************************************************
+     * VARIABLES RELATED FUNCTIONS
+     **********************************************************************/
+    std::vector<variable_info_t>       get_variable_info_list(void);
+    std::vector<requested_variable_t>          get_requested_variables(void);
+    status_t                           set_requested_variables_names( 
std::vector<std::string> variable_names );
+
+    /***********************************************************************
+     * HISTORY RELATED FUNCTIONS
+     **********************************************************************/
+    string_vector      get_history_list(void);
+
+    /*************************************************************************
+     *************************************************************************
+     * FUNCTIONS USED TO ACCESS DATA FROM THE OCTAVE SERVER SIDE
+     *
+     * NOTE: THIS IMPLIES THAT THESE ARE ONLY CALLED FROM
+     * OCTAVE DURING A TIME IN WHICH THINGS ARE KNOWN TO
+     * BE "THREAD-SAFE".  PROPOSED LOCATIONS:
+     *     src/toplev.cc - main_loop() at the end of the do...while
+     *     src/pt-bp.h   - MAYBE_DO_BREAKPOINT just prior to the do_keyboard
+     * Most of these will call octave API functions to "pull" the data, rather
+     * than having octave pass in the data.  This will help make changes 
+     * exlusive to this class if/when the Octave API changes.
+     *************************************************************************
+     *************************************************************************/
+    
+    /**
+     * Calls all the appropriate functions that follow to update Octave
+     * according to the data sent from the client in a thread-safe manner.
+     *
+     * Algorithm:
+     *   Acquire lock
+     *   process_breakpoint_add_remove_modify
+     *   set_current_breakpoint
+     *   set_breakpoint_list
+     *   ...
+     *   Release lock
+     */
+    status_t process_octave_server_data(void);
+ 
+    /***********************************************************************
+     * DEBUGGING RELATED FUNCTIONS
+     **********************************************************************/   
+    status_t set_breakpoint_list(void);
+    status_t set_current_breakpoint(std::string filename, int line_number); // 
duplicate of process_breakpoint_action or helper function???
+    status_t process_breakpoint_add_remove_modify(void);
+    status_t process_breakpoint_action(void);
+
+    /***********************************************************************
+     * VARIABLES INTERROGATION RELATED FUNCTIONS
+     **********************************************************************/
+    status_t set_variable_info_list(void);
+    status_t process_requested_variables(void);
+    
+    /***********************************************************************
+     * HISTORY RELATED FUNCTIONS
+     **********************************************************************/   
 
+    status_t set_history_list(void);
+
+};
+
+int server_rl_event_hook_function(void);
+
+extern octave_server oct_octave_server;
+
+#endif // octave_server_h
+
Index: src/toplev.cc
===================================================================
RCS file: /cvs/octave/src/toplev.cc,v
retrieving revision 1.200
diff -u -b -B -r1.200 toplev.cc
--- src/toplev.cc       31 May 2007 20:23:45 -0000      1.200
+++ src/toplev.cc       15 Jul 2007 02:18:38 -0000
@@ -65,9 +65,11 @@
 #include "parse.h"
 #include "pathsearch.h"
 #include "procstream.h"
+#include "oct-rl-edit.h"
 #include "ov.h"
 #include "pt-jump.h"
 #include "pt-stmt.h"
+#include "server.h"
 #include "sighandlers.h"
 #include "sysdep.h"
 #include "syswait.h"
@@ -203,6 +205,7 @@
   octave_initialized = true;
 
   // The big loop.
+  octave_rl_set_event_hook( &server_rl_event_hook_function );
 
   int retval = 0;
   do