skeeto
commented
5 years ago Nice work. It will be nice to have somewhere better to point people when they ask about reusing elfeed-curl.
Since you're using make-process — something I should have done — you can skip coding-system-for-read and process-connection-type. You're already using :connection-type, so also use :coding. I'm sure you realize this already, but I want to highlight it anyway (especially for anyone else following along): Only the "binary" coding system (or equivalent) makes sense since the input potentially contains multiple different encodings that need to be decoded individually.
Oh, nevermind, I just noticed you're using call-process in the sync version, so I suppose you still need to dynamically bind these variables anyway!
The type of value returned by plz-get (very clever name by the way!) varies depending on the arguments. You can get away with this sort of thing in a dynamically typed language, but it would make no sense at all in a statically typed language. This is a guideline I know I've broken myself, and, when I have, I always regret it later: Dynamically typed programs should still be consistent about its types. I mean this in the deep sense. A function doesn't just consistently return a list, it returns a list of values of a specific type, and so on — the sort of consistently required by static typing. It's good practice.
It looks like you never call plz-error? Make it a hard rule for async that either plz-success or plz-error is called exactly once per request. (One exception: if the curl program is not found it's fine to report such a fatal error directly, e.g. via make-process failing.) This is essential for later managing a queue atop the request function, and it's one of the things url-retrieve gets completely wrong. With url-retrieve, some errors are reported synchronously some of the time depending on the platform (DNS failures, TCP handshake failures, etc.), sometimes callbacks are called multiple times, and sometimes they're never called at all! As you're already aware, it's basically impossible to build anything reliable on top of url-retrieve.
One of the things I enjoy about not making elfeed-curl a general URL fetching library is I get to take some shortcuts. For example, I don't quite fully parse the response headers. Elfeed is the only consumer of those headers, and it doesn't need them to be carefully parsed, so it doesn't matter. But for a general library, you perhaps want to follow through. Technically I believe you're supposed to decode header text values as ISO-8859-1, and perhaps also handle RFC 2047 MIME decoding. (How much does curl handle here already? I don't know, but you can probably rely on the headers being at least syntactically valid.)
It's important for elfeed-curl to support multiple URLs in a single invocation. The goal isn't to reduce the number of curl processes, but to exploit HTTP/1.1 keep-alive when those URLs all belong to the same domain. If you're following multiple feeds hosted on the same site (e.g. a bunch of YouTube channels), only one TCP connection needs to be established to request all those feeds. The story is even better with HTTP/2. (I think? I don't know how well curl employs it.) It puts less stress on the server and is just generally a nice thing to do. The server is also less likely to penalize such a burst of requests if they use HTTP efficiently. The machinery to make this work is a bit complicated as you've seen (random delimiter "tokens"), so you may not think it's worth the trouble for your library, but it is important for some applications, like Elfeed.
Similar arguments can be made for last-modified and etag headers, which reduce the load on servers and is just good netiquette. Elfeed handles these headers at a higher level, and it's probably also out of scope for plz for the same reason. But it's something to consider.
Returning a string of content is certainly simpler, and it's not wrong per se. I prefer the buffer-passing method of elfeed-curl purely for the sake of efficiency. Unlike strings, buffers can be "sliced" using narrowing, so there's less garbage, fewer allocations, and less copying when presenting a narrowed process output buffer to callbacks. In Elfeed's case, the result needs to be stored in a buffer anyway for the XML parser, so strings are an even less attractive option.
alphapapa
@skeeto Hi Chris,
I finally decided to put together a "simple" HTTP library inspired by
elfeed-curl.el. This doesn't implement much yet, but I wonder if you have any feedback on the basic design.Probably the most notable issue at the moment is that the async version calls the callback with a parsed response struct,with the body as a string, rather than allowing the callback to work directly in the response buffer. I'll see about making that more flexible.
Also, unlike your library, it doesn't allow fetching of multiple URLs with a single curl invocation. Do you think that's important enough that I should implement that, or would the complexity not be generally worth it?
Thanks for any feedback.