Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http_server: Ready to support Async 2.0 gem #3842

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

http_server: Ready to support Async 2.0 gem #3842

wants to merge 4 commits into from

Conversation

ashie
Copy link
Member

@ashie ashie commented Aug 3, 2022

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
Copy link
Member Author

ashie commented Aug 3, 2022

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 ashie changed the title http_server: Support Async 2.0 gem http_server: Ready to support Async 2.0 gem Aug 9, 2022
@daipom
Copy link
Contributor

daipom commented Feb 14, 2023

I'm seeing..

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

test/plugin_helper/test_http_server_helper.rb Outdated Show resolved Hide resolved
@daipom
Copy link
Contributor

daipom commented Feb 14, 2023

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
Copy link
Contributor

daipom commented Feb 14, 2023

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
Copy link
Contributor

daipom commented Feb 14, 2023

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

thread_create(title) do
@_http_server.start(notify)
end

@daipom
Copy link
Contributor

daipom commented Feb 14, 2023

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
Copy link
Contributor

daipom commented Feb 14, 2023

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

@ashie ashie marked this pull request as draft February 15, 2023 00:30
@ashie
Copy link
Member Author

ashie commented Feb 15, 2023

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.

@daipom
Copy link
Contributor

daipom commented Feb 15, 2023

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?)

@ashie ashie added this to the v1.17.0 milestone Mar 28, 2023
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Because dependent io-event (v1.0.9) can't build on Windows.

Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
Signed-off-by: Takuro Ashie <ashie@clear-code.com>
@kenhys
Copy link
Contributor

kenhys commented Apr 4, 2024

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

@kenhys
Copy link
Contributor

kenhys commented Apr 4, 2024

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

It should be "Update with rebase".

@stevehipwell
Copy link

It looks like as of v0.65.0 the async-http Gem required async to be >= 2.10.2.

@ashie
Copy link
Member Author

ashie commented Apr 30, 2024

It looks like as of v0.65.0 the async-http Gem required async to be >= 2.10.2.

Thanks notifying it, we'll check it.

@ashie
Copy link
Member Author

ashie commented Apr 30, 2024

This pull request isn't ready for v1.17, we postpone merging this.

@ashie ashie removed this from the v1.17.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work-In-Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants