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

REFACTOR: Use Async and family #467

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
0ec243c
REFACTOR: uses async and family
Ahmedgagan Jun 28, 2021
4b65ae4
DEV: uses relative require paths
Ahmedgagan Jun 28, 2021
236bdc6
DEV: does not daemonize server
Ahmedgagan Jun 28, 2021
49ff9f0
DEV: removes unused variable
Ahmedgagan Jun 28, 2021
2a8f197
DEV: adds method to check OS
Ahmedgagan Jun 28, 2021
94c1824
DEV: Always sync the output
Ahmedgagan Jun 28, 2021
bbf9327
DEV: Daemonises server & changes as suggested
Ahmedgagan Jul 6, 2021
480ff44
DEV: better handle websocket connections
Ahmedgagan Jul 6, 2021
7f37002
DEV: removes unwanted code
Ahmedgagan Jul 6, 2021
9098775
Fix verbose logging
sj26 Jul 13, 2021
0ed0306
Use a message bus
sj26 Jul 13, 2021
d08ba9f
Why does the data store need websockets?
sj26 Jul 13, 2021
1fe62ca
DEV: Opens port before daemonizing server
Ahmedgagan Jul 13, 2021
652f9df
Merge branch 'main' of https://github.com/sj26/mailcatcher into use-a…
Ahmedgagan Jul 13, 2021
a64a914
DEV: Convers Mintest to RSpec
Ahmedgagan Jul 14, 2021
811a3b5
CI: setup chrome & chromedriver in github CI
Ahmedgagan Jul 15, 2021
f28a102
CI: changes chrome & chromedriver setup
Ahmedgagan Jul 15, 2021
8427572
CI: changes actions folder path
Ahmedgagan Jul 15, 2021
45393ce
CI: Removes custom chrome & chromedriver setup
Ahmedgagan Jul 15, 2021
cf4df41
DEV: update smtp.rb as suggested & change dependency order
Ahmedgagan Jul 15, 2021
c35e63a
DEV: Adds SMTP::URLEndpoint spec
Ahmedgagan Jul 15, 2021
27e3aeb
DEV: Resolves merge conflicts
Ahmedgagan Jul 16, 2021
fad3fa2
DEV: adds more test for SMTP::URLEndpoint class
Ahmedgagan Jul 16, 2021
4be30e9
Resolves merge conflicts
Ahmedgagan Jul 17, 2021
ae2e7f8
RSPEC: Removes method and uses let
Ahmedgagan Jul 17, 2021
56e6bfa
DEV: Removes methods from smtp_spec file
Ahmedgagan Jul 17, 2021
865dbb1
DEV: Adds rspec test for SMTP::Protocol::SMTP::Server class
Ahmedgagan Jul 17, 2021
c1d19e0
DEV: Adds rspec for SMTP::Protocol
Ahmedgagan Jul 20, 2021
7f4bd8f
Merge branch 'main' into use-async
Ahmedgagan Jul 20, 2021
6fb8c25
DEV: Removes CHUNKING
Ahmedgagan Jul 20, 2021
a06b7e2
DEV: Linting
Ahmedgagan Jul 20, 2021
adbc161
DEV: fix ruby 3.0 working
Ahmedgagan Jul 22, 2021
ae93635
DEV: always convert command to upcase
Ahmedgagan Jul 26, 2021
eb6f322
Merge branch 'main' into use-async
Ahmedgagan Jul 29, 2021
6815f29
Update spec/smtp_protocol_spec.rb
Ahmedgagan Jul 29, 2021
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
140 changes: 68 additions & 72 deletions lib/mail_catcher.rb
Original file line number Diff line number Diff line change
@@ -1,36 +1,21 @@
# frozen_string_literal: true

# Apparently rubygems won't activate these on its own, so here we go. Let's
# repeat the invention of Bundler all over again.
gem "eventmachine", "1.0.9.1"
gem "mail", "~> 2.3"
gem "rack", "~> 1.5"
gem "sinatra", "~> 1.2"
gem "sqlite3", "~> 1.3"
gem "thin", "~> 1.5.0"
gem "skinny", "~> 0.2.3"
sj26 marked this conversation as resolved.
Show resolved Hide resolved

require 'async/io/address_endpoint'
require 'async/http/endpoint'
require 'async/websocket/adapters/rack'
require 'async/io/shared_endpoint'
require 'falcon'
require "open3"
require "optparse"
require "rbconfig"

require "eventmachine"
require "thin"

module EventMachine
# Monkey patch fix for 10deb4
# See https://github.com/eventmachine/eventmachine/issues/569
def self.reactor_running?
(@reactor_running || false)
end
end

require "mail_catcher/version"
require 'socket'
require 'mail'
require 'mail_catcher/message'
require 'mail_catcher/version'

module MailCatcher extend self
autoload :Bus, "mail_catcher/bus"
autoload :Mail, "mail_catcher/mail"
autoload :Smtp, "mail_catcher/smtp"
autoload :SMTP, "mail_catcher/smtp"
autoload :Web, "mail_catcher/web"

def env
Expand All @@ -55,10 +40,6 @@ def windows?
RbConfig::CONFIG["host_os"] =~ /mswin|mingw/
end

def macruby?
mac? and const_defined? :MACRUBY_VERSION
end

def browseable?
windows? or which? "open"
end
Expand Down Expand Up @@ -197,74 +178,89 @@ def run! options=nil
end

puts "Starting MailCatcher"
puts "==> #{smtp_url}"
puts "==> #{http_url}"

Thin::Logging.debug = development?
Thin::Logging.silent = !development?
Async.logger.level = Logger::DEBUG if options[:verbose]

# One EventMachine loop...
EventMachine.run do
# 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
puts "==> #{smtp_url}"
if options[:daemon]
if quittable?
puts "*** MailCatcher runs as a daemon by default. Go to the web interface to quit."
else
puts "*** MailCatcher is now running as a daemon that cannot be quit."
end
Process.daemon
sj26 marked this conversation as resolved.
Show resolved Hide resolved
end

Async::Reactor.run do |task|
smtp_address = Async::IO::Address.tcp(options[:smtp_ip], options[:smtp_port])
smtp_endpoint = Async::IO::AddressEndpoint.new(smtp_address)
smtp_socket = rescue_port(options[:smtp_port]) { smtp_endpoint.bind }

# 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)
puts "==> #{http_url}"
smtp_endpoint = MailCatcher::SMTP::URLEndpoint.new(URI.parse(smtp_url), smtp_endpoint)
smtp_server = MailCatcher::SMTP::Server.new(smtp_endpoint) do |envelope|
MailCatcher::Mail.add_message(sender: envelope.sender, recipients: envelope.recipients,
source: envelope.content)
end

# Open the web browser before detatching console
if options[:browse]
EventMachine.next_tick do
browse http_url
smtp_task = task.async do |task|
task.annotate "binding to #{smtp_socket.local_address.inspect}"

begin
smtp_socket.listen(Socket::SOMAXCONN)
smtp_socket.accept_each(task: task, &smtp_server.method(:accept))
ensure
smtp_socket.close
end
end

# Daemonize, if we should, but only after the servers have started.
if options[:daemon]
EventMachine.next_tick do
if quittable?
puts "*** MailCatcher runs as a daemon by default. Go to the web interface to quit."
else
puts "*** MailCatcher is now running as a daemon that cannot be quit."
end
Process.daemon
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 daemonisation removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding it in the next commit. Keeping it in the async block was blocking the process instead of daemonisation. I got the solution for this 😅.

http_address = Async::IO::Address.tcp(options[:http_ip], options[:http_port])
http_endpoint = Async::IO::AddressEndpoint.new(http_address)
http_socket = rescue_port(options[:http_port]) { http_endpoint.bind }

http_endpoint = Async::HTTP::Endpoint.new(URI.parse(http_url), http_endpoint)
http_app = Falcon::Adapters::Rack.new(Web.app)
http_server = Falcon::Server.new(http_app, http_endpoint)

task.async do |task|
task.annotate "binding to #{http_socket.local_address.inspect}"

begin
http_socket.listen(Socket::SOMAXCONN)
http_socket.accept_each(task: task, &http_server.method(:accept))
ensure
http_socket.close
end
end

browse(http_url) if options[:browse]
end
rescue Interrupt
# Cool story
end

def quit!
EventMachine.next_tick { EventMachine.stop_event_loop }
Async::Task.current.reactor.stop
end

def http_url
"http://#{@@options[:http_ip]}:#{@@options[:http_port]}#{@@options[:http_path]}"
end

protected
protected

def smtp_url
"smtp://#{@@options[:smtp_ip]}:#{@@options[:smtp_port]}"
end

def http_url
"http://#{@@options[:http_ip]}:#{@@options[:http_port]}#{@@options[:http_path]}"
end

def rescue_port port
begin
yield

# XXX: EventMachine only spits out RuntimeError with a string description
rescue RuntimeError
if $!.to_s =~ /\bno acceptor\b/
puts "~~> ERROR: Something's using port #{port}. Are you already running MailCatcher?"
puts "==> #{smtp_url}"
puts "==> #{http_url}"
exit -1
else
raise
end
rescue Errno::EADDRINUSE
sj26 marked this conversation as resolved.
Show resolved Hide resolved
puts "~~> ERROR: Something's using port #{port}. Are you already running MailCatcher?"
puts "==> #{smtp_url}"
puts "==> #{http_url}"
exit(-1)
end
end
end
7 changes: 0 additions & 7 deletions lib/mail_catcher/bus.rb

This file was deleted.

15 changes: 4 additions & 11 deletions lib/mail_catcher/mail.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
# frozen_string_literal: true

require "eventmachine"
require "json"
require "mail"
require "sqlite3"
require 'async/websocket/client'

module MailCatcher::Mail extend self
def db
Expand Down Expand Up @@ -56,10 +56,7 @@ def add_message(message)
add_message_part(message_id, cid, part.mime_type || "text/plain", part.attachment? ? 1 : 0, part.filename, part.charset, body, body.length)
end

EventMachine.next_tick do
message = MailCatcher::Mail.message message_id
MailCatcher::Bus.push(type: "add", message: message)
end
MailCatcher.send_data(type: 'add', message: message(message_id))
end

def add_message_part(*args)
Expand Down Expand Up @@ -157,18 +154,14 @@ def delete!
@delete_all_messages_query ||= db.prepare "DELETE FROM message"
@delete_all_messages_query.execute

EventMachine.next_tick do
MailCatcher::Bus.push(type: "clear")
end
MailCatcher.send_data(type: 'clear')
end

def delete_message!(message_id)
@delete_messages_query ||= db.prepare "DELETE FROM message WHERE id = ?"
@delete_messages_query.execute(message_id)

EventMachine.next_tick do
MailCatcher::Bus.push(type: "remove", id: message_id)
end
MailCatcher.send_data(type: 'remove', id: message_id)
end

def delete_older_messages!(count = MailCatcher.options[:messages_limit])
Expand Down
18 changes: 18 additions & 0 deletions lib/mail_catcher/message.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module MailCatcher
def send_data(data)
Async do
endpoint = Async::HTTP::Endpoint.parse("#{MailCatcher.http_url}/messages")

begin
Async::WebSocket::Client.connect(endpoint) do |connection|
puts 'Connected...'

connection.write data
connection.flush
end
Copy link
Owner

Choose a reason for hiding this comment

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

What's this bit about, is this constructing a websocket client to itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have used Async Websocket gem which needs to create a client to itself to send data. you can see the example here

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 don’t think we should do that. There should be a way to push a message to clients without being a client. And clients probably shouldn’t be able to send each other messages.

Copy link
Contributor Author

@Ahmedgagan Ahmedgagan Jul 6, 2021

Choose a reason for hiding this comment

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

I did some research and I think there's no way to push a message without the connection object which we get from the connection to WebSocket itself. I improved connection handling in the latest commit

Copy link
Owner

Choose a reason for hiding this comment

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

I've pushed a new version which uses a central bus for broadcasting and ignores client messages as I was describing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just saw that. before I tried using queue but unfortunately I was not getting the connections. Your implementation looks damn good 😍 Thanks for the implementation 😄

rescue Errno::ECONNREFUSED
puts 'Cannot connect to the host. Please try again...'
end
end
end
end
Loading