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.

Sign up for GitHub

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

make it work with ruby 3.1 by upgrading thin to the latest version … #496

Closed
wants to merge 12 commits into from
Closed

make it work with ruby 3.1 by upgrading thin to the latest version … #496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eb03fdf
+ make it work with ruby 3.1 by upgrading thin to the latest version …
Jan 27, 2022
f1e379d
code review changes
Mar 4, 2022
c17f876
Include testing ruby 3.1 on CI
sj26 Mar 4, 2022
74f97dd
Merge branch 'sj26:main' into main
Wanderfalke Mar 6, 2022
f71d348
bring back INT signal handling
Mar 7, 2022
9140a60
add net-http as dependency constant init warnings
Mar 7, 2022
95872aa
trigger ci
Apr 20, 2022
4c07d57
Create main.yml
Wanderfalke Apr 20, 2022
b8197d1
remove workflow
Apr 20, 2022
a122c04
try latest bundler
Apr 20, 2022
13294a0
fix workflow syntax
Apr 20, 2022
544578c
fix workflow syntax
Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ jobs:

strategy:
matrix:
ruby-version: ['2.6', '2.7', '3.0']
ruby-version: ['2.6', '2.7', '3.0', '3.1']

steps:
- uses: actions/checkout@v2

- uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby-version }}
bundler: '2.3.11'
bundler-cache: true

- uses: actions/setup-node@v2
Expand Down
8 changes: 7 additions & 1 deletion lib/mail_catcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,12 @@ def run! options=nil

# One EventMachine loop...
EventMachine.run do
trap("INT") do
EventMachine.add_timer(0) do
Copy link
Owner

Choose a reason for hiding this comment

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

I think EventMachine should already be quitting cleanly. But, if not, I think for this line you want:

Suggested change
EventMachine.add_timer(0) do
EventMachine.next_tick do

Copy link
Author

Choose a reason for hiding this comment

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

You're right. The magic was to pass signals: false to thin.

Copy link
Owner

Choose a reason for hiding this comment

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

But now thin doesn't exit cleanly?

Sorry, by "EventMachine should already be quitting cleanly" I meant that thin's signal handler is enough to shut down the reactor including draining thin's connections cleanly. Did that break? Is that why you made this change initially?

Copy link
Owner

Choose a reason for hiding this comment

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

Ctrl+C now does this for me:

 ▲ ~/Developer/mailcatcher bundle exec mailcatcher --foreground                                                                               Wanderfalke-main 9m ● ⬡
Starting MailCatcher v0.8.1
==> smtp://127.0.0.1:1025
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:206: warning: already initialized constant Net::BufferedIO::BUFSIZE
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:208: warning: previous definition of BUFSIZE was here
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:503: warning: already initialized constant Net::NetPrivate::Socket
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:504: warning: previous definition of Socket was here
==> http://127.0.0.1:1080
^CTraceback (most recent call last):
	20: from /opt/rbenv/versions/2.7.5/bin/bundle:23:in `<main>'
	19: from /opt/rbenv/versions/2.7.5/bin/bundle:23:in `load'
	18: from /opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/libexec/bundle:34:in `<top (required)>'
	17: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/friendly_errors.rb:123:in `with_friendly_errors'
	16: from /opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.1.4/libexec/bundle:46:in `block in <top (required)>'
	15: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli.rb:24:in `start'
	14: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/base.rb:476:in `start'
	13: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli.rb:30:in `dispatch'
	12: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor.rb:399:in `dispatch'
	11: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	10: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	 9: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli.rb:476:in `exec'
	 8: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli/exec.rb:28:in `run'
	 7: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli/exec.rb:63:in `kernel_load'
	 6: from /opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/bundler/cli/exec.rb:63:in `load'
	 5: from /opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/bin/mailcatcher:23:in `<top (required)>'
	 4: from /opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/bin/mailcatcher:23:in `load'
	 3: from /Users/sj26/Developer/mailcatcher/bin/mailcatcher:6:in `<top (required)>'
	 2: from /Users/sj26/Developer/mailcatcher/lib/mail_catcher.rb:180:in `run!'
	 1: from /opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/eventmachine-1.0.9.1/lib/eventmachine.rb:193:in `run'
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/eventmachine-1.0.9.1/lib/eventmachine.rb:193:in `run_machine': Interrupt

Copy link
Author

Choose a reason for hiding this comment

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

My initial motivation for this was the fact, that for me on MacOS 12.1 (Intel) CTRL+C / SIGINT did nothing and mailcatcher process can only be stopped with SIGKILL.
Passing "signals: false" to thin makes SIGINT work, but as you state above also prints stacktrace. Explicitely trapping INT signal in the eventmachine loop makes it exit cleanly.
BTW, your suggestion to use "EventMachine.next_tick" leads to following error: eventmachine.rb:1117:in "synchronize": can't be called from trap context (ThreadError)

quit!
end
end

# Set up an SMTP server to run within EventMachine
rescue_port options[:smtp_port] do
EventMachine.start_server options[:smtp_ip], options[:smtp_port], Smtp
Expand All @@ -187,7 +193,7 @@ def run! options=nil
# Let Thin set itself up inside our EventMachine loop
# (Skinny/WebSockets just works on the inside)
rescue_port options[:http_port] do
Thin::Server.start(options[:http_ip], options[:http_port], Web)
Thin::Server.start(options[:http_ip], options[:http_port], Web, signals: false)
puts "==> #{http_url}"
end

Expand Down
37 changes: 20 additions & 17 deletions lib/mail_catcher/web/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@
require "uri"

require "sinatra"
require "skinny"
require "faye/websocket"

require "mail_catcher/bus"
require "mail_catcher/mail"

class Sinatra::Request
include Skinny::Helpers
end
Faye::WebSocket.load_adapter("thin")

module MailCatcher
module Web
Expand Down Expand Up @@ -61,21 +59,26 @@ def asset_path(filename)
end

get "/messages" do
if request.websocket?
request.websocket!(
:on_start => proc do |websocket|
bus_subscription = MailCatcher::Bus.subscribe do |message|
begin
websocket.send_message(JSON.generate(message))
rescue => exception
MailCatcher.log_exception("Error sending message through websocket", message, exception)
end
env = request.env
if Faye::WebSocket.websocket?(env)
bus_subscription = nil

ws = Faye::WebSocket.new(env)
ws.on(:open) do |_|
bus_subscription = MailCatcher::Bus.subscribe do |message|
begin
ws.send(JSON.generate(message))
rescue => exception
MailCatcher.log_exception("Error sending message through websocket", message, exception)
end
end
end

websocket.on_close do |*|
MailCatcher::Bus.unsubscribe bus_subscription
end
end)
ws.on(:close) do |_|
MailCatcher::Bus.unsubscribe(bus_subscription) if bus_subscription
end

ws.rack_response
else
content_type :json
JSON.generate(Mail.messages)
Expand Down
10 changes: 6 additions & 4 deletions mailcatcher.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ Gem::Specification.new do |s|
"views/**/*",
] - Dir["lib/mail_catcher/web/assets.rb"]
s.require_paths = ["lib"]
s.executables = ["mailcatcher", "catchmail"]
s.extra_rdoc_files = ["README.md", "LICENSE"]
s.executables = %w[mailcatcher catchmail]
s.extra_rdoc_files = %w[README.md LICENSE]

s.required_ruby_version = ">= 2.6.0"

Expand All @@ -37,8 +37,10 @@ Gem::Specification.new do |s|
s.add_dependency "rack", "~> 1.5"
s.add_dependency "sinatra", "~> 1.2"
s.add_dependency "sqlite3", "~> 1.3"
s.add_dependency "thin", "~> 1.5.0"
s.add_dependency "skinny", "~> 0.2.3"
s.add_dependency "thin", "~> 1.8.0"
s.add_dependency "net-smtp", "~> 0.3.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Why was net-smtp added here?

Copy link
Contributor

Choose a reason for hiding this comment

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

net-smtp

net-smtp has been extracted from the default gems in Ruby 3.1

Ref:

Copy link
Author

Choose a reason for hiding this comment

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

As already mentioned by @tagliala, as of Ruby 3.1, net-smtp was moved to bundled gems.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see. Thanks.

Copy link
Owner

@sj26 sj26 Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This now spits a lot of warnings during startup for me on Ruby 2.7.5:

$ bundle exec mailcatcher --foreground
Starting MailCatcher v0.8.1
==> smtp://127.0.0.1:1025
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:66: warning: already initialized constant Net::ProtocRetryError
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:68: warning: previous definition of ProtocRetryError was here
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:206: warning: already initialized constant Net::BufferedIO::BUFSIZE
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:208: warning: previous definition of BUFSIZE was here
/opt/rbenv/versions/2.7.5/lib/ruby/2.7.0/net/protocol.rb:503: warning: already initialized constant Net::NetPrivate::Socket
/opt/rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/net-protocol-0.1.2/lib/net/protocol.rb:504: warning: previous definition of Socket was here

If it's loading stdlib and the gem, that might cause issues? How does one avoid this double loading on older rubies?

Copy link
Author

Choose a reason for hiding this comment

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

Tried ruby 2.7.5 and I'm getting same warnings. Not sure how to avoid that yet, will keep looking for a solution.

Copy link
Author

Choose a reason for hiding this comment

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

Following this issue: ruby/net-imap#16 , adding net-http as dependency solves "already initialized constant" issues with net-protocol.

s.add_dependency "net-http", "~> 0.2.0"
s.add_dependency "faye-websocket", "~> 0.11.1"

s.add_development_dependency "capybara"
s.add_development_dependency "capybara-screenshot"
Expand Down