bluegreen-labs / ecmwfr

Interface to the public ECMWF API Web Services
https://bluegreen-labs.github.io/ecmwfr/
Other
106 stars 30 forks source link

Wrong version in curl config? #130

Closed ryangooch closed 5 months ago

ryangooch commented 5 months ago

I stumbled upon this while looking for something else. It looks like the code intends for the use of the HTTP/2 protocol, based on the comment, but I believe that the code is actually specifying HTTP/1.1.

    # force http/2 for all products
    httr::set_config(httr::config(http_version = 2))

Link: https://github.com/bluegreen-labs/ecmwfr/blob/534e98831c0af9859bcc70a48258d804f2d62108/R/zzz.R#L142

Somewhat confusingly, it seems the curl uses an enum for specifying that http_version argument. My read could be wrong, but the curl headers source code indicates that the value of 2 corresponds to HTTP/1.1, while a value of 3 would indicate HTTP/2.

I've been tracking this issue in an unrelated project, and I am by no means a curl expert. I do have some experience with numerical weather prediction, though, which is part of what caught my eye when I saw a search hit for this line in your code.

khufkens commented 5 months ago

This is the version which "works". The comment might not be correct in this sense. I would leave as is unless strong counter indications.

ryangooch commented 5 months ago

Sounds good. In my experience, using HTTP/1.1 is less prone to weird curl errors, among other fail cases, so doing what works and using the simpler method, as you say, is better.

The comment and the unintuitive enumeration caught my eye so I just wanted to share what I'd learned at what has most recently bitten me 😅 and try to prevent it biting someone else.