On Sun, Apr 8, 2018 at 7:02 AM, Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:

> This patch adds new golang module in order to bootstrap Golang
> applications on OSv. Its role is to pass golang shared object
> specific main function name - GoMain - and terminate all lingering
> Golang application threads once main thread completes.
>
> To that end two new arguments with default values have been
> added to application constructor and application::run* static methods
> that allow overriding main function name and specifying post-main
> function.
>
> Finally this patch closes the list of patches necessary for full
> Golang support on OSv.
>
> Fixes #850
> Fixes #522
>

I lost track, don't we have one lingering patch with the syscall stack to
fix #522?


> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
>  core/app.cc               | 24 ++++++++++++++++++------
>  include/osv/app.hh        | 17 ++++++++++++++---
>  modules/golang/.gitignore |  3 +++
>  modules/golang/Makefile   | 38 ++++++++++++++++++++++++++++++++++++++
>  modules/golang/go.cc      | 47 ++++++++++++++++++++++++++++++
> +++++++++++++++++
>  5 files changed, 120 insertions(+), 9 deletions(-)
>  create mode 100644 modules/golang/.gitignore
>  create mode 100644 modules/golang/Makefile
>  create mode 100644 modules/golang/go.cc
>
> diff --git a/core/app.cc b/core/app.cc
> index d9a4c6a..17513f4 100644
> --- a/core/app.cc
> +++ b/core/app.cc
> @@ -125,9 +125,12 @@ shared_app_t application::run(const
> std::vector<std::string>& args)
>  shared_app_t application::run(const std::string& command,
>                        const std::vector<std::string>& args,
>                        bool new_program,
> -                      const std::unordered_map<std::string, std::string>
> *env)
> +                      const std::unordered_map<std::string, std::string>
> *env,
> +                      const std::string& main_function_name,
> +                      void (*post_main)())
>


I'm not sure I understood why post_main() is needed. Why can't the user
wait for the application to finish (join()ing it) and then run this
post_main() function?

If we must have this post_main function, I think we better use
std::function instead of a function pointer, it's much more versetile.



>  {
> -    auto app = std::make_shared<application>(command, args, new_program,
> env);
> +    auto app = std::make_shared<application>(command, args, new_program,
> env,
> +                                             main_function_name,
> post_main);
>      app->start();
>      apps.push(app);
>      return app;
> @@ -137,9 +140,12 @@ shared_app_t application::run_and_join(const
> std::string& command,
>                        const std::vector<std::string>& args,
>                        bool new_program,
>                        const std::unordered_map<std::string, std::string>
> *env,
> -                      waiter* setup_waiter)
> +                      waiter* setup_waiter,
> +                      const std::string& main_function_name,
> +                      void (*post_main)())
>  {
> -    auto app = std::make_shared<application>(command, args, new_program,
> env);
> +    auto app = std::make_shared<application>(command, args, new_program,
> env,
> +                                             main_function_name,
> post_main);
>      app->start_and_join(setup_waiter);
>      return app;
>  }
> @@ -147,13 +153,16 @@ shared_app_t application::run_and_join(const
> std::string& command,
>  application::application(const std::string& command,
>                       const std::vector<std::string>& args,
>                       bool new_program,
> -                     const std::unordered_map<std::string, std::string>
> *env)
> +                     const std::unordered_map<std::string, std::string>
> *env,
> +                     const std::string& main_function_name,
> +                     void (*post_main)())
>      : _args(args)
>      , _command(command)
>      , _termination_requested(false)
>      , _runtime(new application_runtime(*this))
>      , _joiner(nullptr)
>      , _terminated(false)
> +    , _post_main(post_main)
>  {
>      try {
>          elf::program *current_program;
> @@ -204,7 +213,7 @@ application::application(const std::string& command,
>          throw launch_error("Failed to load object: " + command);
>      }
>
> -    _main = _lib->lookup<int (int, char**)>("main");
> +    _main = _lib->lookup<int (int, char**)>(main_function_name.c_str());
>      if (!_main) {
>          _entry_point = reinterpret_cast<void(*)()>(_lib->entry_point());
>      }
> @@ -318,6 +327,9 @@ void application::main()
>          _entry_point();
>      }
>
> +    if(_post_main) {
> +        _post_main();
> +    }
>      // _entry_point() doesn't return
>

If _entry_point() doesn't return, how is _post_main() ever called? :-)


>  }
>
> diff --git a/include/osv/app.hh b/include/osv/app.hh
> index 73a1b38..2399228 100644
> --- a/include/osv/app.hh
> +++ b/include/osv/app.hh
> @@ -98,12 +98,16 @@ public:
>       * \param args Parameters which will be passed to program's main().
>       * \param new_program true if a new elf namespace must be started
>       * \param env pointer to an unordered_map than will be merged in
> current env
> +     * \param main_function_name name of the main function - the default
> is 'main'
> +     * \param post_main optional function to be executed after before
> application::main() ends
>

"before"? Maybe after?


>       * \throw launch_error
>       */
>      static shared_app_t run(const std::string& command,
>              const std::vector<std::string>& args,
>              bool new_program = false,
> -            const std::unordered_map<std::string, std::string> *env =
> nullptr);
> +            const std::unordered_map<std::string, std::string> *env =
> nullptr,
> +            const std::string& main_function_name = "main",
> +            void (*post_main)() = nullptr);
>
>      static void join_all() {
>          apps.join();
> @@ -112,7 +116,9 @@ public:
>      application(const std::string& command,
>              const std::vector<std::string>& args,
>              bool new_program = false,
> -            const std::unordered_map<std::string, std::string> *env =
> nullptr);
> +            const std::unordered_map<std::string, std::string> *env =
> nullptr,
> +            const std::string& main_function_name = "main",
> +            void (*post_main)() = nullptr);
>
>      ~application();
>
> @@ -136,13 +142,17 @@ public:
>       * \param args Parameters which will be passed to program's main().
>       * \param new_program true if a new elf namespace must be started
>       * \param env pointer to an unordered_map than will be merged in
> current env
> +     * \param main_function_name name of the main function - the default
> is 'main'
> +     * \param post_main optional function to be executed after before
> application::main() ends
>       * \throw launch_error
>       */
>      static shared_app_t run_and_join(const std::string& command,
>              const std::vector<std::string>& args,
>              bool new_program = false,
>              const std::unordered_map<std::string, std::string> *env =
> nullptr,
> -            waiter* setup_waiter = nullptr);
> +            waiter* setup_waiter = nullptr,
> +            const std::string& main_function_name = "main",
> +            void (*post_main)() = nullptr);
>
>      /**
>       * Installs a termination callback which will be called when
> @@ -243,6 +253,7 @@ private:
>      std::shared_ptr<application_runtime> _runtime;
>      sched::thread* _joiner;
>      std::atomic<bool> _terminated;
> +    void (*_post_main)();
>      friend struct application_runtime;
>  };
>
> diff --git a/modules/golang/.gitignore b/modules/golang/.gitignore
> new file mode 100644
> index 0000000..96b135a
> --- /dev/null
> +++ b/modules/golang/.gitignore
> @@ -0,0 +1,3 @@
> +*.so
> +obj/*
> +*.manifest
> diff --git a/modules/golang/Makefile b/modules/golang/Makefile
> new file mode 100644
> index 0000000..4a6dc2c
> --- /dev/null
> +++ b/modules/golang/Makefile
> @@ -0,0 +1,38 @@
> +INCLUDES = -I. -I../../include -I../../arch/$(ARCH) -I../.. \
> +        -I../../build/$(mode)/gen/include
> +
> +autodepend = -MD -MT $@ -MP
> +CXXFLAGS  = -g -rdynamic -Wall -std=c++11 -fPIC $(INCLUDES) $(autodepend)
> +src = ../..
> +arch = x64
>

Do you need this? Above you use $(ARCH) in uppercase, for example. And you
don't set mode, for example. Maybe you don't need these lines at all.


> +
> +#boost-libs := -lboost_system -lboost_filesystem
> +
> +# the build target executable:
> +TARGET = go
> +CPP_FILES := $(TARGET).cc
> +OBJ_FILES := $(addprefix obj/,$(CPP_FILES:.cc=.o))
> +DEPS := $(OBJ_FILES:.o=.d)
> +
> +LIBS = -lpthread # $(boost-libs) $(DEPEDNDS_LIBS)
> +
> +quiet = $(if $V, $1, @echo " $2"; $1)
> +very-quiet = $(if $V, $1, @$1)
> +
> +$(TARGET).so: $(OBJ_FILES)
> +       $(call quiet, $(CXX) $(CXXFLAGS) -shared -o $(TARGET).so $^
> $(LIBS), LINK $@)
> +
> +obj/%.o: %.cc
> +       $(call quiet, $(CXX) $(CXXFLAGS) -c -o $@ $<, CXX $@)
> +
> +init:
> +       @echo "  MKDIRS"
> +       $(call very-quiet, mkdir -p obj)
> +.PHONY: init
> +
> +module: init $(TARGET).so
> +       echo '/go.so: $${MODULE_DIR}/go.so' > usr.manifest
> +
> +clean:
> +       rm -f $(TARGET)*.so usr.manifest
> +       $(call very-quiet, $(RM) -rf obj)
> diff --git a/modules/golang/go.cc b/modules/golang/go.cc
> new file mode 100644
> index 0000000..0bb1986
> --- /dev/null
> +++ b/modules/golang/go.cc
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright (C) 2018 Waldemar Kozaczuk
> + *
> + * This work is open source software, licensed under the terms of the
> + * BSD license as described in the LICENSE file in the top-level
> directory.
> + */
> +#include <osv/app.hh>
> +
> +// This tiny app acts as a front-end to Golang applications running on
> OSv.
> +// In essence it starts new application to execute specified Golang shared
> +// object. The spawn application is setup to terminate all remaining
> Golang
> +// runtime threads after main thread exits.
> +
> +using namespace osv;
> +
> +// As described in https://github.com/golang/go/issues/11100 some
> +// Golang runtime threads do not terminate automatically when
> +// main thread exits. This is very similar to JVM shutdown
> +// behavior.
> +// The stop_all_remaining_golang_threads() function stops
> +// all threads associated with the new Golang app started and
> +// gets passed as post-main function below.
> +// This call is unsafe, in the sense we assume that those
> +// renegade threads are not holding any critical resources
> +// (e.g., not in the middle of I/O or memory allocation).
> +static void stop_all_remaining_golang_threads()
> +{
> +    while(!application::unsafe_stop_and_abandon_other_threads()) {
> +        usleep(100000);
> +    }
> +}
> +
> +extern "C"
> +int main(int argc, char **argv)
> +{
> +    const std::string go_program(argv[1]);
> +
> +    std::vector<std::string> go_program_arguments;
> +    for(auto i = 2; i < argc; i++ )
> +        go_program_arguments.push_back(argv[i]);
> +    //
> +    // Setup new app with Golang specific main function name - GoMain -
> and
> +    // post main function that will terminate all remaining Golang
> runtime threads
> +    auto app = application::run(go_program, go_program_arguments, false,
> nullptr,
> +                                "GoMain", stop_all_remaining_golang_
> threads);
> +    return app->join();
>

So I think my suggestion above holds:
I think we should run app->join(), and *then* call
stop_all_remaining_golang_threads().
Then we won't need this "post_main" feature at all.

+}
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to