fluentd icon indicating copy to clipboard operation
fluentd copied to clipboard

http_server: Ready to support Async 2.0 gem

Open ashie opened this issue 3 years ago • 15 comments

Which issue(s) this PR fixes: Fixes #

What this PR does / why we need it: http_server plugin helper doesn't work with Async 2.x gem due to using obsolete usage. This PR updates it to follow current documented way: https://github.com/socketry/async-http/blob/0a65acd7cf7486e1877f0da86580e1692cd8207b/readme.md#usage

It's applicable both Async 2.x & Async 1.x.

But this PR still stay on Async 1.x because io-event gem which is required by Async 2.x can't build on Windows.

Docs Changes: None

Release Note: Same with the title

ashie avatar Aug 03 '22 04:08 ashie

Hmm, can't build io-event on Windows: https://github.com/fluent/fluentd/runs/7645045887?check_suite_focus=true

Gem::Ext::BuildError: ERROR: Failed to build gem native extension.

current directory:
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/gems/3.1.0/gems/io-event-1.0.9/ext
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/bin/ruby.exe -I
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/3.1.0 -r
./siteconf20220803-6988-hlcmr.rb extconf.rb
checking for rb_ext_ractor_safe()... yes
checking for &rb_fiber_transfer()... yes
checking for -luring... no
checking for sys/epoll.h... no
checking for sys/event.h... no
checking for sys/eventfd.h... no
checking for rb_io_descriptor()... yes
checking for &rb_process_status_wait()... no
checking for rb_fiber_current()... yes
checking for &rb_fiber_raise()... yes
checking for ruby/io/buffer.h... yes
creating extconf.h
creating Makefile

current directory:
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/gems/3.1.0/gems/io-event-1.0.9/ext
make.exe DESTDIR\= clean

current directory:
C:/hostedtoolcache/windows/Ruby/3.1.2/x64/lib/ruby/gems/3.1.0/gems/io-event-1.0.9/ext
make.exe DESTDIR\=
generating IO_Event-x64-mingw-ucrt.def
compiling ./io/event/event.c
compiling ./io/event/selector/selector.c
./io/event/selector/selector.c:26:10: fatal error: sys/wait.h: No such file or
directory
   26 | #include <sys/wait.h>
      |          ^~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:246: selector.o] Error 1

make failed, exit code 2

ashie avatar Aug 03 '22 05:08 ashie

I'm seeing..

daipom avatar Feb 14 '23 04:02 daipom

Hmm, this log is not outputted. I don't know why...

[debug]: #0 fluent/log.rb:309:debug:   0.0s: Async::IO::Socket
      | Binding to #<Addrinfo: 0.0.0.0:24220 TCP>

In master, when launching Fluentd with this config, the log is outputted using ConsoleAdapter.

<source>
 @type monitor_agent
</source>

daipom avatar Feb 14 '23 09:02 daipom

https://github.com/socketry/async-io/blob/v1.34.3/lib/async/io/socket.rb#L157

At this point, somehow Console.logger is Console::Terminal::Logger, not Fluent::Log::ConsoleAdapter.

I'm thinking about the possibility that the logger is not passed to the child task correctly.

daipom avatar Feb 14 '23 10:02 daipom

It seems that Console.logger = Fluent::Log::ConsoleAdapter.wrap(@logger) is not applied to this thread.

https://github.com/fluent/fluentd/blob/bf7498d6ff2805bfa517bbd6f7c17103c7d879d4/lib/fluent/plugin_helper/http_server.rb#L74-L76

daipom avatar Feb 14 '23 10:02 daipom

Console.logger= sets the value as Fiber::local::instance. This is why Console.logger = Fluent::Log::ConsoleAdapter.wrap(@logger) is not applied to other threads.

https://github.com/socketry/console/blob/main/lib/console.rb#L13-L19 https://github.com/socketry/console/blob/main/lib/console/logger.rb#L19-L20 https://github.com/socketry/fiber-local/blob/main/lib/fiber/local.rb#L34-L53

daipom avatar Feb 14 '23 10:02 daipom

We may have to do something like this... Maybe there is a more correct way...

--- a/lib/fluent/plugin_helper/http_server/server.rb
+++ b/lib/fluent/plugin_helper/http_server/server.rb
@@ -40,7 +40,6 @@ module Fluent
           @uri = URI("#{scheme}://#{@addr}:#{@port}").to_s
           @router = Router.new(default_app)
           @server_task = nil
-          Console.logger = Fluent::Log::ConsoleAdapter.wrap(@logger)
 
           opts = if tls_context
                    { ssl_context: tls_context }
@@ -55,10 +54,12 @@ module Fluent
         end
 
         def start(notify = nil)
+          Console.logger = Fluent::Log::ConsoleAdapter.wrap(@logger)
           @logger.debug("Start async HTTP server listening #{@uri}")
 
           Async do |task|
             @server_task = task.async do
+              Console.logger = Fluent::Log::ConsoleAdapter.wrap(@logger)
               @server.run
             end
             if notify

daipom avatar Feb 14 '23 10:02 daipom

We may have to do something like this... Maybe there is a more correct way...

Thanks for checking it. It seems there is no other way to replace them, so I applied the fix. But we don't need to rush merging this. I've reverted the status of this PR to draft. We'll check it later.

ashie avatar Feb 15 '23 00:02 ashie

Note: We have to consider the possibility of Async::HTTP::Server::run() creating child tasks inside. (If so, how should we apply our logger to the child tasks' threads?)

daipom avatar Feb 15 '23 03:02 daipom

Hmm, GitHub "update branch" feature is not what I want. rebased manually.

kenhys avatar Apr 04 '24 07:04 kenhys

Hmm, GitHub "update branch" feature is not what I want. rebased manually.

It should be "Update with rebase".

kenhys avatar Apr 04 '24 07:04 kenhys