Address ECONNRESET in issue #118 #165

pm-mck · 2021-10-03T15:52:04Z

Summary

I took a few liberties here, so please feel free to recommend alternate strategies for any of the following:

I enabled Cram tests in the dune-project. I felt that cram tests would be the best way to make the TCP RST packet issue pop up. I generally like how this turned out, however I don't really like how it does open TCP port selection.
I added a few dependencies in the dream.opam file, such as emile and mimic. I personally run Dream's build in an esy "harness" of sorts, and it was not happy about those undeclared dependencies. Not a major issue to remove if this adds too much clutter or is undesired.
The place where I added the fix seems like it is awfully naive to the structure of Dream's error handling. The Cram test itself validates the fix, (although I do need to test a bit more broadly, only osx/x64 so far, and I haven't tested ipv4 yet) so I'm happy with it as a one-off but I think there likely should be some sort of designated "error mangler" function for these types of things.

What has been done?

Write a test that demonstrates the behavior in ipv6
Write a test that demonstrates the behavior in ipv4
Update the error logging "middleware" to identify ECONNRESETs and downgrade to `Info
Test OSX
Test Linux
Test Windows if I can

…to run cram tests; add a cram test for aantron#118; propose a small fix for aantron#118

aantron · 2021-10-17T00:04:16Z

dream.opam

@@ -55,6 +55,7 @@ depends: [
  "conf-libev" {os != "win32"}
  "cstruct" {>= "6.0.0"}
  "dune" {>= "2.7.0"}  # --instrument-with.
+  "emile"


AFAIK Dream should not depend on emile at all. What is your setup in some more detail, and what did you observe that caused adding emile here?

I generally use esy to build ocaml projects, almost as a force of habit. I believe I was building "everything", including tests, examples, etc. and esy was complaining about emile. I went back with a new switch and ran the opam install --deps-only ./dream.opam --with-test which worked. I will pull back this dependency from the PR.

aantron · 2021-10-17T00:05:27Z

dream.opam

@@ -64,9 +65,12 @@ depends: [
  "lwt_ssl"
  "logs" {>= "0.5.0"}
  "magic-mime"
+  "mimic"


This should be moved to dream-mirage.opam, which does seem to depend directly on mimic (and the dependency is not listed). Does this work with your setup if it is moved?

aantron · 2021-10-17T00:05:49Z

dream.opam

  "mirage-clock"
  "mirage-crypto" {>= "0.8.1"}  # AES-256-GCM.
  "mirage-crypto-rng" {>= "0.8.0"}  # Signature of initialize.
+  "mirage-time"
+  "mirage-stack"


These should probably also be moved to dream-mirage.opam.

aantron · 2021-10-17T00:14:14Z

src/http/error_handler.ml

-    backtrace |> Dream__middleware.Log.iter_backtrace (fun line ->
-      select_log error.severity (fun log ->
-        log ?request:error.request "%s" line))
+  (* TODO is this the right place to handle lower level connection resets? *)


I'm not yet sure where this should be handled. Going to discuss a bit in #118, and do some research.

aantron · 2021-10-17T00:16:29Z

test/cram/dune

@@ -0,0 +1,2 @@
+(cram


Rather than a cram test, could this be done using an in-process expect test? It seems like that approach would be more portable, as it wouldn't depend on Unix commands, and could be a bit less awkward, since it wouldn't require any complex shell command lines (everything would be kept in OCaml code).

I spent some time today on this, I wanted to report back and see if you have any thoughts on a different approach.

Short answer is that it is possible, but without modifying Dream itself to add test hooks into the http module, quite ugly.

I have a successful expect test which does 3 things:

Spins up a server on an open port using Dream.serve with a stop promise.

Continually runs a "reset client" which tries to connect to the server. If it connects, it immediately closes the socket fd which causes an RST packet to be sent to the server. It then resolves the stop promise.

Those two promises race against a third timeout promise - if the "reset client" doesn't resolve the stop promise in 4 seconds, the whole thing gets torn down.

A few things I don't like:

The test has to ignore SIGPIPE since closing the fd the way it does signals a closed pipe.

There is a timer since there's a potential the server can get stuck due to other failures, causing the test to stall indefinitely.

Since the server actually binds to an open TCP port, and we don't know when that binding succeeds, there's retry logic in the "reset client." This is kind of painful.

Reviewing the expect tests got me thinking: how useful would it be to offer a test capability to be able to inject errors at various points in Dream's request processing cycle to validate their behaviors?

how useful would it be to offer a test capability to be able to inject errors at various points in Dream's request processing cycle to validate their behaviors?

Potentially very useful. I have a big project/roadmap category for making Dream apps (and probably Dream itself) really testable. But it's in the queue behind several other things.

I think it's fine then to use a cram test. If really good programmatic Dream testing materializes, I can change the test to use that later, if necessary.

aantron · 2021-10-17T00:27:25Z

src/http/error_handler.ml

+  (* TODO is this the right place to handle lower level connection resets? *)
+  | `Exn (Unix.Unix_error (Unix.ECONNRESET, _, _)) ->
+     let condition = `String "Connection Reset at Client" in
+     let severity = `Info in


@hannesm Would it be fine for your use case if the errors were reported as warnings, as already done for most other client errors?

sure. at the same time with an operations view, I'd expect if warning and error level occur, I should check the log and look how to fix the issues. OTOH if a random client resets the TCP connection or sends a malformed request, that should be on the informational level. but eventually that should be discussed in a different issue and applied uniformally.

pm-mck added 2 commits October 3, 2021 10:20

add a few missing dependencies to dream.opam; allow the dune-project …

93fe295

…to run cram tests; add a cram test for aantron#118; propose a small fix for aantron#118

allow econnreset to identify an open port

e6bdf76

pm-mck mentioned this pull request Oct 3, 2021

When the remote peer hangs up, a stash of errors are reported (on the error level in the log) #118

Open

aantron reviewed Oct 17, 2021

View reviewed changes

aantron force-pushed the master branch from 74efe19 to 3eb6f38 Compare March 2, 2023 12:56

aantron added 2 commits November 12, 2023 19:01

Document Dream.run ?socket_path

78cf84a

Merge branch 'master' into aantron#118-handle-ECONNRESET

7df24a2

aantron force-pushed the master branch 2 times, most recently from ce104e4 to a9a9dfc Compare November 14, 2023 09:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address ECONNRESET in issue #118 #165

Address ECONNRESET in issue #118 #165

pm-mck commented Oct 3, 2021

aantron Oct 17, 2021

pm-mck Oct 19, 2021

aantron Oct 17, 2021

aantron Oct 17, 2021

aantron Oct 17, 2021

aantron Oct 17, 2021

pm-mck Oct 23, 2021

aantron Nov 8, 2021

aantron Oct 17, 2021

hannesm Oct 18, 2021

Address ECONNRESET in issue #118 #165

Are you sure you want to change the base?

Address ECONNRESET in issue #118 #165

Conversation

pm-mck commented Oct 3, 2021

Summary

What has been done?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment