REFACTOR: Use Async and family #467

Ahmedgagan · 2021-06-28T09:27:35Z

Hi @sj26
This PR is on behalf of Discourse.
This PR replaces Eventmachine with Async.

SamSaffron · 2021-07-06T01:47:06Z

Howdy @sj26, anything we can do to help drive this forward? Do you need any assistance reviewing, I could ask another person from our team to carefully review.

sj26 · 2021-07-06T02:26:45Z

Hi folks! This looks amazing, thanks for the contribution. I haven't had capacity to review this properly yet, and the changes are very extensive. It also looks like there are a few functional changes in here beyond refactoring which will need to be considered. I'll see if I can start dropping some smaller comments. (And I was leaving time for the "more changes" 🙂)

sj26 · 2021-07-06T02:27:13Z

lib/mail_catcher.rb

-          else
-            puts "*** MailCatcher is now running as a daemon that cannot be quit."
-          end
-          Process.daemon


Why was daemonisation removed?

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 😅.

lib/mail_catcher/web/application.rb

sj26 · 2021-07-06T02:29:07Z

lib/mail_catcher/web/application.rb

+      def initialize
+        super
+        @connections = Set.new
+        @semaphore = Async::Semaphore.new(512)


Is this supervising websockets and acting as a message bus? Perhaps that's a separate concern from the core web application, and should be extracted?

I'm not averse to pulling in something like MessageBus to do this well, if there's dependency alignment. Or building an equivalent little bus in here using async. I've been meaning to generalise MailCatcher's websocket-y message bus to keep better state between the backend and frontend.

I removed the semaphores, it was not of use for our application. I think Async-WebSockets do well for our case. thoughts?

sj26 · 2021-07-06T02:31:06Z

lib/mail_catcher/smtp.rb

+      def run
+        @endpoint.accept(&method(:accept))
+      end
+    end


This is amazing, a whole SMTP implementation! If we're cooking our own then I think we'll need to add some testing.

(Ah yes, it looks familiar: e77365b)

Yes, I used your async branch and made it work with the master branch's latest changes 😀

Great!

We'll need to test the implementation. How would you like to do that? If minitest feels cubersome then I'm happy to switch to rspec if it's more familiar.

Yes, I am more familiar with RSpec, I'll start with converting to RSpec tests

sj26 · 2021-07-06T02:32:40Z

lib/mail_catcher/message.rb

+
+          connection.write data
+          connection.flush
+        end


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

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

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.

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

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

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 😄

lib/mail_catcher.rb

Ahmedgagan · 2021-07-12T06:25:45Z

Hi @sj26
Last week I made some commits as per your suggestions, Do have a look when you get time.

lib/mail_catcher.rb

mailcatcher --verbose failed with: NameError: uninitialized constant MailCatcher::Logger Async.logger is a Console::Logger, not a Logger. But it accepts symbols as log levels.

Add a Bus which is a Channel modelled roughly after eventmachine channels which can broadcast a message to multiple consumers as async tasks. Then modify each websocket to use a Queue to copy broadcast messages to clients.

sj26 · 2021-07-13T02:54:41Z

lib/mail_catcher.rb

+      @http_endpoint = Async::IO::AddressEndpoint.new(@http_address)
+      @http_socket = rescue_port(options[:http_port]) { @http_endpoint.bind }
+      puts "==> #{http_url}"
+    end


Yeah, nice.

sj26 · 2021-07-13T05:01:34Z

I've merged a switch to GitHub Actions to main so that there is some form of CI:

#469

I'm not set on using mintest or even selenium (https://github.com/rubycdp/cuprite looks great!) but the fundamental infrastructure is available (ruby, node, chrome, etc).

…sync

Ahmedgagan · 2021-07-14T19:17:12Z

Hi @sj26
I have added a commit to convert Mintest to RSpec and All tests are passed(unless skipped) in my local. Starting to write SMTP implementation spec.

Also, If you wish to check Github CI setup works well with RSpec, Just approve this PR and GitHub actions will be performed on all my next commits

sj26 · 2021-07-15T01:34:08Z

lib/mail_catcher/smtp.rb

+                  write_response 501, 'Unexpected parameters or arguments'
+                  next
+                end
+                envelope = nil


Suggested change

envelope = nil

@state.clear

Ahmedgagan · 2021-07-19T05:57:52Z

Hi @sj26 ,
I have added RSpec for smtp.rb file, if possible do have a look

sj26 · 2021-07-20T01:53:20Z

Thanks @Ahmedgagan. The formatting of that file is a bit random, tabs and spaces etc, but I'll peer between that for the moment.

I can see it's testing some of the plumbing of the line protocol stuff. I'm not so interested in that, and more interested in testing that we're doing correct SMTP protocol interactions. Like can a new connection EHLO and receive an expected response. Does the MAIL FROM: ... command parse the sender correctly, and return expected responses in various cases. Does the server enforce a correct ordering of EHLO, MAIL FROM, RCPT TO, DATA and error correctly when done out of order. Do the encodings work — does it support 7bit transfer correctly, and utf8. Because this is a completely novel SMTP implementation I think these details are important. [edit: testing the dot stuffing behaviour is super important too, the . behaviour in DATA]

By SMTP clients I do indeed mean testing it manually from different environments, not just ruby's Net::SMTP. Like does it work from Django, or from Laravel, or other environments where devs are likely to use it. I agree that releasing a beta is a good way to do that too, but I don't want to drop such a beta without doing at least a little bit of independent testing first.

I didn't test if (m)any clients use BDAT before implementing it. If we can achieve all the required client support without it then I would consider removing it to simplify the implementation and testing. I think I only added it while playing with an SMTP server implementation and exploring ways to ingest emails with full octet encodings which have historically been a pain in MailCatcher's side. [edit: see later comment, I think we should remove CHUNKING/BDAT.]

So, a roadmap to getting this merged:

I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.
This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.
We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.
We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

sj26 · 2021-07-20T05:05:39Z

Net::SMTP is the ruby defacto
SwiftMailer's ESMTP seems the popular PHP smtp client
Django uses smtplib from Python's standard library
Golang has a standard smtp package

Most of them seem to support 8BITMIME and SMTPUTF8.

None support CHUNKING (BDAT) so I think we should rip that out.

Ahmedgagan · 2021-07-22T11:53:40Z

Hi @sj26
I made it work with ruby 3.0, and also added some more spec to test SMTP protocol interactions. Do share your thoughts.

Now, I am starting with testing it in different frameworks.

Ahmedgagan · 2021-07-26T06:25:01Z

Hi @sj26
I tested the SMTP implementation in python, Ruby, Golang & PHP. It worked in all 4 perfectly, there was a small problem with python in which it was sending commands in lowercase & our switch case was comparing uppercase strings. This commit solves it. I tested with sending plain text, HTML, attachments, attaching images in mail content, etc.

Ahmedgagan · 2021-07-27T08:06:37Z

So, a roadmap to getting this merged:

I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.

This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.

We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.

We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

Hi @sj26
I think I have completed points 2,3,4. Can you have a look?

spec/smtp_protocol_spec.rb

sj26 · 2021-07-28T03:32:02Z

Thanks for testing it across implementations @Ahmedgagan!

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get 0.8.0 out and stable with the improvements which had been collecting on the main branch.

Ahmedgagan · 2021-07-29T06:17:52Z

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

ooh, thanks for the efforts 😅

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which
aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I'll also try to write some more concrete tests.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get
0.8.0 out and stable with the improvements which had been collecting on the main branch.

That's great, after the release we will merge those stable changes in this branch.

Co-authored-by: Samuel Cochran <[email protected]>

Ahmedgagan · 2021-08-09T05:12:44Z

I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

Hi @sj26
How are you? Can you have a look at the PR?

SamSaffron · 2021-08-24T01:36:07Z

Howdy @sj26 hope you are doing well! Anything we can do to move the merge forward.

We already had multiple people on ARM macs test this and it appears to be working nicely!

SamSaffron · 2021-09-30T22:40:36Z

@sj26 any news? Do you need a hand with the gem, I can ask around if any of our senior devs is open to helping maintain

sj26 · 2021-10-12T02:13:14Z

@Ahmedgagan @SamSaffron sorry folks, I haven't had capacity to come back and do the project managementy bits. The world is not kind at the moment. I love this work, but I want it to land nicely and remain maintainable.

I'd like some more feature tests to make sure it's not breaking anything fundamental that folks currently use (working on main, and continuing to work on this branch), more test coverage of the novel smtp implementation's edges to make sure it doesn't regress, and I'd like it to match the code style a little more (it's silly but I'd prefer to keep to double quotes like the existing code, for example).

I think there is also a depth of edges which need careful review. For example, I think the new quit implementation currently just stops the reactor cold instead of sending a quit message to clients and ending the reactor after current connections are closed and finished (which the eventmachine implementation used to do by pushing to the bus and then stopping on the next tick). It needs a careful comb through.

If you have capacity to do these things, or know folks who can, that would be grand. Otherwise I'll get to it when I can.

Fixes NixOS#96908 Switching to fork that uses async library, this fixes darwin builds. sj26/mailcatcher#467

Ahmedgagan added 6 commits June 28, 2021 11:33

REFACTOR: uses async and family

0ec243c

DEV: uses relative require paths

4b65ae4

DEV: does not daemonize server

236bdc6

DEV: removes unused variable

49ff9f0

DEV: adds method to check OS

2a8f197

DEV: Always sync the output

94c1824

sj26 reviewed Jul 6, 2021

View reviewed changes

lib/mail_catcher/web/application.rb Outdated Show resolved Hide resolved

sj26 reviewed Jul 6, 2021

View reviewed changes

lib/mail_catcher.rb Outdated Show resolved Hide resolved

DEV: Daemonises server & changes as suggested

bbf9327

Ahmedgagan commented Jul 6, 2021

View reviewed changes

lib/mail_catcher.rb Show resolved Hide resolved

Ahmedgagan added 2 commits July 6, 2021 18:25

DEV: better handle websocket connections

480ff44

DEV: removes unwanted code

7f37002

sj26 reviewed Jul 12, 2021

View reviewed changes

lib/mail_catcher.rb Show resolved Hide resolved

sj26 and others added 4 commits July 13, 2021 11:38

Fix verbose logging

9098775

mailcatcher --verbose failed with: NameError: uninitialized constant MailCatcher::Logger Async.logger is a Console::Logger, not a Logger. But it accepts symbols as log levels.

Use a message bus

0ed0306

Add a Bus which is a Channel modelled roughly after eventmachine channels which can broadcast a message to multiple consumers as async tasks. Then modify each websocket to use a Queue to copy broadcast messages to clients.

Why does the data store need websockets?

d08ba9f

DEV: Opens port before daemonizing server

1fe62ca

sj26 reviewed Jul 13, 2021

View reviewed changes

Ahmedgagan added 2 commits July 13, 2021 11:50

Merge branch 'main' of https://github.com/sj26/mailcatcher into use-a…

652f9df

…sync

DEV: Convers Mintest to RSpec

a64a914

sj26 reviewed Jul 15, 2021

View reviewed changes

Ahmedgagan added 4 commits July 17, 2021 12:18

Resolves merge conflicts

4be30e9

RSPEC: Removes method and uses let

ae2e7f8

DEV: Removes methods from smtp_spec file

56e6bfa

DEV: Adds rspec test for SMTP::Protocol::SMTP::Server class

865dbb1

Ahmedgagan added 5 commits July 20, 2021 16:09

DEV: Adds rspec for SMTP::Protocol

c1d19e0

Merge branch 'main' into use-async

7f4bd8f

DEV: Removes CHUNKING

6fb8c25

DEV: Linting

a06b7e2

DEV: fix ruby 3.0 working

adbc161

DEV: always convert command to upcase

ae93635

sj26 reviewed Jul 28, 2021

View reviewed changes

spec/smtp_protocol_spec.rb Outdated Show resolved Hide resolved

Merge branch 'main' into use-async

eb6f322

Update spec/smtp_protocol_spec.rb

6815f29

Co-authored-by: Samuel Cochran <[email protected]>

sj26 linked an issue Nov 1, 2021 that may be closed by this pull request

vulnerability in rack #488

Closed

sj26 mentioned this pull request Mar 2, 2022

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

Closed

thenonameguy added a commit to thenonameguy/nixpkgs that referenced this pull request Dec 1, 2022

mailcatcher: 0.7.1 -> 1.0.0.beta1

700a393

Fixes NixOS#96908 Switching to fork that uses async library, this fixes darwin builds. sj26/mailcatcher#467

thenonameguy mentioned this pull request Dec 1, 2022

mailcatcher: 0.7.1 -> 1.0.0.beta1 NixOS/nixpkgs#203942

Closed

13 tasks

sj26 mentioned this pull request Jul 29, 2023

gemspec - update eventmachine, rack, sinatra, sqlite3 #546

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REFACTOR: Use Async and family #467

REFACTOR: Use Async and family #467

Ahmedgagan commented Jun 28, 2021 •

edited

Loading

SamSaffron commented Jul 6, 2021

sj26 commented Jul 6, 2021 •

edited

Loading

sj26 Jul 6, 2021

Ahmedgagan Jul 6, 2021

sj26 Jul 6, 2021 •

edited

Loading

Ahmedgagan Jul 6, 2021

sj26 Jul 6, 2021

sj26 Jul 6, 2021 •

edited

Loading

Ahmedgagan Jul 6, 2021

sj26 Jul 12, 2021

Ahmedgagan Jul 12, 2021

sj26 Jul 6, 2021

Ahmedgagan Jul 6, 2021

sj26 Jul 6, 2021

Ahmedgagan Jul 6, 2021 •

edited

Loading

sj26 Jul 13, 2021

Ahmedgagan Jul 13, 2021

Ahmedgagan commented Jul 12, 2021

sj26 Jul 13, 2021

sj26 commented Jul 13, 2021 •

edited

Loading

Ahmedgagan commented Jul 14, 2021

sj26 Jul 15, 2021

Ahmedgagan commented Jul 19, 2021

sj26 commented Jul 20, 2021 •

edited

Loading

sj26 commented Jul 20, 2021 •

edited

Loading

Ahmedgagan commented Jul 22, 2021

Ahmedgagan commented Jul 26, 2021 •

edited

Loading

Ahmedgagan commented Jul 27, 2021

sj26 commented Jul 28, 2021 •

edited

Loading

Ahmedgagan commented Jul 29, 2021

Ahmedgagan commented Aug 9, 2021 •

edited

Loading

SamSaffron commented Aug 24, 2021

SamSaffron commented Sep 30, 2021

sj26 commented Oct 12, 2021

REFACTOR: Use Async and family #467

Are you sure you want to change the base?

REFACTOR: Use Async and family #467

Conversation

Ahmedgagan commented Jun 28, 2021 • edited Loading

SamSaffron commented Jul 6, 2021

sj26 commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sj26 Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sj26 Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ahmedgagan Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ahmedgagan commented Jul 12, 2021

Choose a reason for hiding this comment

sj26 commented Jul 13, 2021 • edited Loading

Ahmedgagan commented Jul 14, 2021

Choose a reason for hiding this comment

Ahmedgagan commented Jul 19, 2021

sj26 commented Jul 20, 2021 • edited Loading

sj26 commented Jul 20, 2021 • edited Loading

Ahmedgagan commented Jul 22, 2021

Ahmedgagan commented Jul 26, 2021 • edited Loading

Ahmedgagan commented Jul 27, 2021

sj26 commented Jul 28, 2021 • edited Loading

Ahmedgagan commented Jul 29, 2021

Ahmedgagan commented Aug 9, 2021 • edited Loading

SamSaffron commented Aug 24, 2021

SamSaffron commented Sep 30, 2021

sj26 commented Oct 12, 2021

Ahmedgagan commented Jun 28, 2021 •

edited

Loading

sj26 commented Jul 6, 2021 •

edited

Loading

sj26 Jul 6, 2021 •

edited

Loading

sj26 Jul 6, 2021 •

edited

Loading

Ahmedgagan Jul 6, 2021 •

edited

Loading

sj26 commented Jul 13, 2021 •

edited

Loading

sj26 commented Jul 20, 2021 •

edited

Loading

sj26 commented Jul 20, 2021 •

edited

Loading

Ahmedgagan commented Jul 26, 2021 •

edited

Loading

sj26 commented Jul 28, 2021 •

edited

Loading

Ahmedgagan commented Aug 9, 2021 •

edited

Loading