artifice icon indicating copy to clipboard operation
artifice copied to clipboard

Limit activation to a specific host/port combination

Open halostatue opened this issue 12 years ago • 4 comments

The modifications in these change sets provide the ability to limit activation to a specific host/port combination instead of universally overriding Net::HTTP.

I've implemented this because of a specific need that I have in production where an app was developed as an API endpoint and a "web" endpoint intended to run on different ports, but performance considerations have forced us to push them into a single application, so I can use artifice to still use the separate API without requiring network overhead, yet I will still get correct behaviour from the APIs that I must still reach over the network.

halostatue avatar Sep 24 '11 02:09 halostatue

I've considered the merits of this as well. Ultimately, I've found it so convenient to handle things like that inside of the endpoint I create that I've come to see little need for such a feature.

I think the motivation is sensible enough, but ultimate (to me) this feature would add unnecessary complexity as part of the core library.

Have you thought about making this an extension?

I don't know if this is helpful, but an extension exists that enables you to allow a real http request to slip through your endpoint. https://github.com/remi/artifice-passthru

That's my $0.02

theconektd avatar Sep 27 '11 17:09 theconektd

artifice-passthru is useful and essentially does the same as #activate_for, but it's also a bit late for my production use case. What I don't like about aritifice-passthru is threefold:

  1. It's sort of the inverse of what I'm doing here, at least in the examples given.
  2. It's substantially more complex than what I'm doing here.
  3. It's fragile. The changes that I've made are much less fragile because they completely fall back to the standard behaviour unless the endpoint is matched.

The complexity and fragility are big arguments against artifice-passthru and the "extension" approach, IMO. Compared to what I've provided, artifice-passthru has to keep a thread-local variable on a per-request basis to rescue the case when the block calls #passthru!, and it takes a perfectly good Net::HTTPResponse and turns it into a Rack response just so that it can be turned into another Net::HTTPResponse. Outbound it also creates a new HTTPRequest rather than forcing 'super' behaviour on the existing request. Those new objects and transformations add a pretty hefty overhead that isn't necessary.

The changes I've done to add #host/#port to the #endpoint tracking appear to me to be in the spirit of what Artifice is doing (single-level artifice override), and would be fairly easy to extend if core-Artifice was modified to support either stacked Artifice endpoints or multiple Artifice endpoints.

I'll admit that this may not be the best way to implement this behaviour, but I think that something a little more invasive to the core is probably the safest and fastest way to let Artifice work with both real and fake calls, and I've tried very hard to make the minimal number of changes. (The biggest change I could probably dump is the #use_rack_endpoint? method, as Net::HTTP uses the @newimpl variable for much the same sort of dispatch change in-line.)

halostatue avatar Sep 29 '11 04:09 halostatue

That's fine. Either way, perhaps this would be more appropriate as a plugin?

If it's especially painful to write extensions for artifice, perhaps we should focus on fixing that? I like the idea of keeping the core as simple as possible and focusing on providing robust extensibility if that's what's needed.

Thoughts?

theconektd avatar Sep 29 '11 23:09 theconektd

OK, let's talk plug-in capability and efficiency. The core piece of functionality missing from Artifice is the ability to do an efficient dispatch to something other than the singular Rack endpoint provided on activation. This affects different users in different ways. Some users want the dispatch to be to multiple Rack endpoints; I want (and the person who wrote artifice-passthru wants) the dispatch to be to a real Net::HTTP connection.

  1. I don't have a particular need for multiple endpoint dispatch, but I can imagine mechanisms to return the endpoint required for a particular request; it would need to be a modification such that rack_request = RackRequest.new(self.class.endpoint) doesn't necessarily call self.class.endpoint, but an extension point. Perhaps a method #endpoint would be the way to make this pluggable.

    def endpoint
     self.class.endpoint
    end
    
  2. On the other hand, the mechanism that I require needs something just a little more invasive. The only way to efficiently do this is pretty much what I've done: a modification to #connect and #request that will call the parent class method (through super) if the connection is not to be handled by the Rack endpoint but the standard Net::HTTP mechanisms. This, too, could at least partially be handled by the #endpoint method, but I don't believe that this is the best choice for this.

I can try to work this up as a different patch set if you're interested.

halostatue avatar Sep 30 '11 02:09 halostatue