Nadav,
Thanks for reviewing the patch. Please see my responses below.
On Sunday, April 15, 2018 at 12:36:31 PM UTC-4, Nadav Har'El wrote:
>
>
> On Sun, Apr 8, 2018 at 7:02 AM, Waldemar Kozaczuk <jwkoz...@gmail.com
> <javascript:>> 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?
>
Technically this patch is enough to have golang-example work. Not enough to
have non-trivial examples like golang-httpserver work. But I am ambivalent.
>
>
>> Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com <javascript:>>
>> ---
>> 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?
>
That is exactly what I tried first but it did not work. I think it happens
because as you described in #850 even though GoMain returns some children
threads of golang runtime do NOT. And therefore app->join() never returns.
So that is why stop_all_remaining_golang_threads has to be called in the
right after GoMain exits in the same thread.
Do you think it can be achieved in a better more elegant way?
>
> 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.
>
I can try.
>
>
>
>> {
>> - 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? :-)
>
What specific scenario are you talking about? In general? Because in case
of golang GoMain does return.
>
>
>> }
>>
>> 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?
>
Sure.
>
>
>> * \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.
>
>
Will look into it.
> +
>> +#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.
>
> Please see my explanation above.
> +}
>> --
>> 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+u...@googlegroups.com <javascript:>.
>> 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.
