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 #4619

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

Watson1978
Copy link
Contributor

@Watson1978 Watson1978 commented Aug 28, 2024

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:

Release Note:

@ioquatix
Copy link

How are you using cool.io? Can you replace it with Async?

@ioquatix
Copy link

By the way, I'm sorry that this is complicated, I did not intend for this to become a big problem. I'm happy to advise or help out.

@ashie
Copy link
Member

ashie commented Aug 28, 2024

How are you using cool.io? Can you replace it with Async?

Thanks for your comment!
We have an idea to migrate to Async for quite some time:
https://github.com/fluent/fluentd/wiki/Roadmap-and-Development-tasks#v1
but we haven't started migrating it yet because Fluentd depends on cool.io deeply, it's our fundamental event loop mechanism.

We may need to start considering it now.

@ioquatix
Copy link

Is it exposed to user code or is it all internal implementation?

@ashie
Copy link
Member

ashie commented Aug 29, 2024

Is it exposed to user code or is it all internal implementation?

Partially wrapped by our API but Coolio's classes are exposed to plugins.
Probably many built-in & third-party plugins use them.
e.g.)

class StatWatcher < Coolio::StatWatcher
def initialize(path, log, &callback)
@callback = callback
@log = log
super(path)
end
def on_change(prev, cur)
@callback.call
rescue
@log.error $!.to_s
@log.error_backtrace
end
end
class TimerTrigger < Coolio::TimerWatcher
def initialize(interval, log, &callback)
@log = log
@callback = callback
super(interval, true)
end
def on_timer
@callback.call
rescue => e
@log.error e.to_s
@log.error_backtrace
end
end

@ioquatix
Copy link

I see, it's a tricky change then.

One option would be to change cool.io to rename IO::Buffer to it's own namespace, e.g. cool.io::IOBuffer (which would make more sense anyway).

@Watson1978
Copy link
Contributor Author

Watson1978 commented Oct 2, 2024

Rebased with master in order to run CI with cool.io gem v1.9.0
Related to socketry/cool.io#82 (comment)

@ioquatix
Copy link

ioquatix commented Oct 2, 2024

Ah, we also need to fix io-event compiling on Windows, I'll sort it out.

@Watson1978
Copy link
Contributor Author

Ah, we also need to fix io-event compiling on Windows, I'll sort it out.

@ioquatix If you have a time, I want you will take an action for socketry/io-event#112 🙏🏻

ashie and others added 7 commits October 4, 2024 09:59
Signed-off-by: Takuro Ashie <[email protected]>
Signed-off-by: Shizuo Fujita <[email protected]>
Because dependent io-event (v1.0.9) can't build on Windows.

Signed-off-by: Takuro Ashie <[email protected]>
Signed-off-by: Shizuo Fujita <[email protected]>
Signed-off-by: Takuro Ashie <[email protected]>
Signed-off-by: Shizuo Fujita <[email protected]>
This reverts commit 895fa6b.

Signed-off-by: Shizuo Fujita <[email protected]>
@Watson1978 Watson1978 removed the pending To be done in the future label Oct 4, 2024
@Watson1978 Watson1978 marked this pull request as ready for review October 4, 2024 01:47
@daipom daipom self-requested a review October 4, 2024 01:54
@daipom daipom added this to the v1.18.0 milestone Oct 4, 2024
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

I've tried it with in_monitor_agent, it works fine for me.
Thanks a lot!

@ashie ashie merged commit b4814cb into fluent:master Oct 9, 2024
16 checks passed
@Watson1978 Watson1978 deleted the async-2.x branch October 9, 2024 01:06
@ioquatix
Copy link

ioquatix commented Oct 9, 2024

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants