Docs: HTTP Client Tutorial

[moigagoo]

No description provided.

[moigagoo]

@arnetheduck I've finished the tutorial. It took me much longer than I had expected but at least I am now sure I do understand how to do HTTP requests with Chronos.

[moigagoo]

@arnetheduck the CI is failing but it doesn't seem to have anything to do with the docs.

[moigagoo]

Since it's a non-code PR I'm not going to wait for a formal approval and just merge it on Feb 20.

[arnetheduck]

Since it's a non-code PR I'm not going to wait for a formal approval and just merge it on Feb 20.

Since this ends up being a recommendation for how to use chronos, let's wait until it's been reviewed - ie before we commit it, we should also probably take a look at whether we can simplify the http API itself so that the tutorial itself becomes more simple, ie let's use this PR as a way to talk about those simplifications.

[moigagoo]

Since this ends up being a recommendation for how to use chronos, let's wait until it's been reviewed

OK, sure, let's do that.

before we commit it, we should also probably take a look at whether we can simplify the http API itself so that the tutorial itself becomes more simple, ie let's use this PR as a way to talk about those simplifications.

IMHO simplifying the API and improving the docs could be done independently as parts of a bigger process: update the docs, collect feedback, improve the API, update the docs, etc.

I.e., the docs could accurately reflect the current state and that would be valuable per se. They also could inspire some changes in the API but that's a different independent value.

I always get nervous when I see a PR depending on another PR, I'm scared that the scope will grow out of control and the task will take more time than it should... But maybe I'm overreacting.

Anyway, I'm trusting you with the way to treat this PR, all above is just an opinion.

[arnetheduck]

could be done independently

It's a bit like introducing a feature and discovering that the codebase needs refactoring - we tend to extract the refactoring and do that first so that the feature can be kept clean, using the feature as a driver and motivator for the refactoring.

In any case, I expect both docs and API updates to go out in the next release together, so it's not far off anyway - that, and even without http api changes there are things to address in this tutorial (though a proper review needs a bit of time) - users anxious to get going can simply look at the branch in its current state and get started already.

[arnetheduck]

better catch the AsyncSemaphoreError here, no point leaking it

[moigagoo]

Done.

[moigagoo]

@arnetheduck I've rewritten some chapters significantly and added a couple more.

[arnetheduck]

resolve addresses

#656 moves name resolution to the connect attempt which indeed may block - what's needed here is to rewrite the resolver in chronos to use async versions of name resolution - this is an interesting problem to solve but it's somewhat messy to do well - ie there are more or less 3 ways to approach it:

• platform-specific code, fall back on sync for random platforms - this is probably the best starting point (like the zig example above)
• thread pool - messy lifetime issues
• wrap c-ares or similar

Potentially, this could be an extension point where the dispatcher could have an extension mechanism for custom resolvers.

[arnetheduck]

this function is deprecated as of #656 - in fact, this page can probably be removed for now and replaced with a warning box in the previous page - the longer-term solution here is that chronos needs to implement async name resolution.

the advice to "pre-resolve" is also problematic since DNS records change - it's usually not correct to resolve only once at the startup of the application, specially in a monitoring application. For now, highlighting that it might happen is sufficient and we'll come back to the problem when chronos has an async resolver

[arnetheduck]

like everything else, the body reader must be closed.

an important aside here is also that if you don't read the whole body (either via readOnce/read or consume) the connection will effectively be broken and cannot be reused (since the server is still sending bytes on the same tcp connection). for a monitoring app this is fine, but it's something that should be called out.

[moigagoo]

Done. I wonder though, how does one determine what needs to be closed? Open connections are closed, that makes sense. But that a reader needs closing is not so obvious.

[arnetheduck]

But that a reader needs closing is not so obvious.

pretty much everything in chronos needs closing since we're dealing with OS resources (sockets) - a reader is a stream -> streams need closing

a reader needs closing for example because that signals to the runtime that reading is done that no more reading will be done using that particular reader - when you call finish on the response (also missing, now that I look at the code), chronos can know that it's safe to reuse the connection for the next request - or that it should be disconnected.

[moigagoo]

an important aside here is also that if you don't read the whole body (either via readOnce/read or consume) the connection will effectively be broken and cannot be reused (since the server is still sending bytes on the same tcp connection). for a monitoring app this is fine, but it's something that should be called out.

I've added request closing in finally, will that be a more hygienic way to finish working with the request and free the connection properly?

[arnetheduck]

will that be a more hygienic way to finish working with the request and free the connection properly?

It depends on what you want to achieve. When you use a body reader, it reads as many bytes as you asked for but the server will keep sending. Consider what happens when you ask for a 100 bytes file and only "read" 50 bytes - 50 bytes will still be queued on the socket and waiting there for someone to read them. If you were to try to reuse the connection at this point, there would be 50 bogus bytes belonging to the previous request, so the best thing chronos can do at that point is close the connection and not reuse it.

The body reader, although it can be used for partial downloads, is mainly meant for streaming "whole" requests, not just for reading parts - for reading parts, http 1.1 doesn't really have a good mechanism except https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Range_requests which is useful mostly for static content.

When you don't read all the bytes of the body, the best thing chronos can do is close the connection and open a new connection for the next request.

If you end up read all bytes using the body reader, you should also call finish(response). This is mirrored for the request as well, ie when you're making a request with a big body (such as when uploading a file using POST), you stream the body then call finish(request) to get the response.

So a fully streamed request/response looks something like:

let 
  req = HttpClientRequestRef.new(...)
  resp = 
    try:
      let writer = req.open()
      try:
        await writer.write(...)
      finally:
        await writer.closeWait()
      await req.finish() # On happy path only
    finally:
      await req.closeWait() # Always close

try:
  let reader = resp.getBodyReader()
  try: 
    let data = reader.read()
    ...
    await resp.finish() # On happy path only
  finally:
    await reader.closeWait() # Always close
finally:
  await resp.closeWait() # Always close

[arnetheduck]

I guess this brings us to another point: for an uptime monitor, instead of reading "parts of" the body, the normal thing to do would be to use a HEAD request (which gives the headers without the content) - doesn't really matter for this PR, but a future update to the tutorial could use a HEAD for the discovery but then you'd have to figure out a way to demonstrate body readers .. maybe by streaming some "result" page to a log file, where the entire body gets read.

[moigagoo]

I rewrote the parts with request and response initialization and the findMaker proc.

Request, response, and body reader are now closed after use and response us finished if we managed to read the full response (i.e. len(buffer) == 0).

IMO the request and response creation now look very good. To me, the best sign is when I can read the code top to bottom as prose.

I noticed that I can't specify exception in except within try block in let: 3946f0d#diff-afdd5453693afd6c94de6c3761053aab468f293a392e91f67d2b596fa13f97b5R71

If I replace this line with except HttpError, AsyncTimeoutError (the two exception that can actually be raise here), the code won't compile:

      Info: using /home/moigagoo/.nimble/pkgs2/nim-2.2.10-17ec440fdb89f8903db29a17898af590087d2b64/bin/nim for compilation
   Building uptimemon/uptimemon using c backend
 Nim Output assertions.nim(38)       raiseAssert
        ... Error: unhandled exception: closureiters.nim(335, 17) `c[i].kind == nkType`  [AssertionDefect]
       Tip: 6 messages have been suppressed, use --verbose to show them.
vnext.nim(1128)          buildFromDir

    Error:  Build failed for the package: uptimemon

This looks like a bug but maybe I'm missing something?

[arnetheduck]

Suggested change

	echo message
	echo &"[NOK] {address.hostname}{address.path}: {response.status}

strformat can help make these less verbose

[moigagoo]

I think introducing new import and new syntax here would bring more confusion that benefit.

[moigagoo]

Especially the & syntax is weird for the uninitialized. fmt is better but still is something new and unrelated to the topic.

[moigagoo]

Since the chapter about preemptive DNS resolution is no more more and we're back to checking raw strings instead of HttpAddresses, these bits are now less noisy.

[arnetheduck]

a sleep like this is usually a good time to bring up drift - ie the uptime monitor, as written, will not check once per 10 seconds - instead, it will check once per 10s + check time + timer delay where timer delay is the time it takes for the system to wake up and "resume" execution .. if the runtime is busy / blocked, this can take some time.

A drift-free version of this is to do something like (exact logic needs checking):

var next = Moment.now() + 10.seconds
while true:
  await process()
  let now = Moment.now()
  await sleepAsync(max(now, next) - now)
  next = max(now, next + 10.seconds)

[moigagoo]

AFAIU this code won't guarantee that the next check happens exactly 10 seconds after the previous one; in fact, nothing can. It just guarantees that we won't wait for another 10 seconds if the scheduled time has already passed.

IMO it isn't critical if the check runs a little later. I've added a note about drift to chapter 7.

[arnetheduck]

LGTM as soon as the final touchups are done, solid base for continuing the docs work!