Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intrinsic for making HTTP requests #8347

Closed
gshuflin opened this issue Sep 27, 2019 · 4 comments
Closed

Intrinsic for making HTTP requests #8347

gshuflin opened this issue Sep 27, 2019 · 4 comments

Comments

@gshuflin
Copy link
Contributor

We'd like to implement an engine intrinsic that allows pants @rules to make arbitrary HTTP requests in a non-blocking way.

Open questions:
-what should the API for this look like? (I'm currently thinking two types HttpRequest and HttpResponse, both of which have reasonable-looking fields on them).
-how exactly should this be implemented in the engine? Is UrlToDownload a good guide?

@stuhood
Copy link
Member

stuhood commented Sep 27, 2019

As discussed briefly in slack: I think that we should be very, very cautious as we choose to add more non-deterministic/side-effecting APIs. The ones that currently exist are:

  1. @console_rule (likely side-effecting)
  2. ExecuteProcessRequest (likely non-deterministic, rarely side-effecting)

This is not to say that we definitely shouldn't do this: just that we should be conscious of the surface area of code that will be complex.

One example: UrlToFetch might look analogous here, but because it requires a digest, it is deterministic and "pretty unlikely" to be side-effecting. That means that we can safely retry it, and it means we never have to expose failure, timeouts, retries to the @rule author (ie, it doesn't need to be falliable).

@benjyw
Copy link
Contributor

benjyw commented Sep 27, 2019

We can probably ignore side-effecting (i.e., POST requests) for now. But in general rules need to be able to access APIs.

@stuhood
Copy link
Member

stuhood commented Jan 9, 2020

stuhood added a commit that referenced this issue Feb 15, 2020
### Problem

The rust level `Node::cacheable` flag is currently only used to mark `@goal_rule`s as uncacheable (because they are allowed to operate on `@sideeffecting` types, such as the `Console` and the `Workspace`). But since the implementation of `cacheable` did not allow it to operate deeply in the Graph, we additionally needed to mark their parent `Select` nodes uncacheable, and could not use the flag in more positions.

Via #7350, #8495, #8347, and #8974, it has become clear that we would like to safely allow nodes deeper in the graph to be uncacheable, as this allows for the re-execution of non-deterministic processes, or re-consumption of un-trackable state, such as:
1. a process receiving stdin from a user
2. an intrinsic rule that pokes an un-watched file on the filesystem
3. interacting with a stateful process like git

Note that these would all be intrinsic Nodes: it's not clear that we want to expose this facility to `@rule`s directly.

### Solution

Finish adding support for uncacheable nodes. Fixes #6598.

When an uncacheable node completes, it will now keep the value it completed with (in order to correctly compute a `Generation` value), but it will re-compute the value once per `Session`. The accurate `Generation` value for the uncacheable node allows its dependents to "clean" themselves and not re-run unless the uncacheable node produced a different value than it had before. 

### Result

The `Node::cacheable` flag may be safely used deeper in the graph, with the semantics that requests for any of an uncacheable node's dependents will cause it to re-run once per `Session`. The dependents will not re-run unless the value of the uncacheable node changes (regardless of the `Session`).
@stuhood
Copy link
Member

stuhood commented Dec 17, 2020

Building upon the lack of side-effects, the Graph has grown more and more support for transparent retry, cancellation, etc. At this point, I think it's clear that an HTTP API to make "arbitrary" requests would not be safe, and that any HTTP API would need to assume idempotent requests.

I think that this ticket is also stale, so will close.

@stuhood stuhood closed this as completed Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants