Ambiguity in asynchronous execution setting

[Temikus]

While figuring out acceptance tests for vagrant-google, I found a weird logic piece in the implementation of synchronous operations in Fog. As an example, let's take a look at Server class' destroy method:

def destroy(async=true)
  requires :name, :zone

  data = service.delete_server(name, zone_name)
  operation = Fog::Compute::Google::Operations.new(:service => service).get(data.body['name'], data.body['zone'])
  unless async
    operation.wait_for { ready? }
  end
  operation
end

The async parameter is just a true/false switch, so if we need to perform the operation synchronously (important for tests for example), we need to specify it like so:

instance.destroy(false)

Which, I find highly confusing to understand for someone who's reading the code later. Due to the default being true, it is not easy to wrap around with a statement, since it will not make any sense either:

async_execution = false
instance.destroy(async_execution)

I was wandering - maybe it makes sense to make it a named parameter? This will allow for:

1. Logically sound statements:

instance.start(async: false)

2. Ability to write in more execution flow control parameters easily if we need them.

[ikehz]

@Temikus I like this a lot; I've noticed this as well. However, I don't believe your proposed solution is possible until we've dropped support for Ruby 1. Until then, we'd have to take an optional hash argument instead of using a named parameter. Fog has no plans to drop support for Ruby 1.9, (only 1.8,) as far as I know.

@plribeiro3000 is there precedent for this in other fog projects?

[ikehz]

Regardless of implementation, this is a breaking change, so I've marked it for v1.0.0.

[plribeiro3000]

@ihmccreery we are following the main repo conventions. I believe this would give us less issues like a provider that supports a ruby version that the main one does not.

For now we will have to keep all 1.8.7 compatible but in the next big release we will drop it.

[ikehz]

@plribeiro3000 Sorry, that wasn't clear. I know we need to keep support for 1.8.7 until fog v2. I meant to ask you about precedent for sync/async parameters for operations in other providers.

[plribeiro3000]

Oh, gotcha. I'm not sure about conventions on this one but usually @geemus always ask to drop boolean params that change the behavior of the method. I tend to agree and define 2 methods with proper names to indicate the exact behavior of each one.

Does that answer your question? (Sorry for the miss on the first attempt)

[geemus]

Yeah, tricky. I think as @plribeiro3000 suggests, we could add async_destroy and sync_destroy or something to help make it really clear. What do you guys think? We could just leave the existing behavior on the existing method that way, but still win some improvement. Thoughts?

[Temikus]

I agree with separating into methods. It seems better than implementing hash option fetching. However, what about the existing method? Will it eventually produce a deprecation warning? Or async_destroy and sync_destroy will be left as helper methods until 1.9.3 will be deprecated (it's bound to happen at some point in the future, even though it may be quite remote).

[ikehz]

Hm. Separate methods are generally a good thing, I think, but it does mean we won't be able to wrap blocks in sync or async rules. Anything like this would no longer be easy:

async_execution = false
instance.destroy(async_execution)

Whatever we decide, I think it makes sense to see a deprecation warning in v0.1.0, and full removal of the current behavior in v1.0.0, which hopefully would sync with fog v2.

[Temikus]

But that's what I was saying. async_execution = false reads like "this is not an asynchronous operation". With the async meaning "not synchronous" logically, it starts to read really confusing when we go to the method and I cannot see a good wrapper around it. Since it's false, it will always be the opposite of what the block is labeled, as with your example:

async_execution = false
instance.destroy(async_execution)

It reads "set the async_execution to false", "destroy instance with async execution". Which is super confusing.

If we had the sync which amounted to true, however, it would still be a bit less readable, but would actually make sense logically, as in:

sync_execution = true
instance.destroy(sync_execution)

or

override_default_behavior = true
instance.destroy(override_default_behavior)

[ikehz]

Yeah, I see what you're saying. Would just a simple variable rename, like

sync_execution_switch = false
instance.destroy(sync_execution_switch)

or, in params world

sync_execution_switch = false
instance.destroy(:async => sync_execution_switch)

solve the issue? I'm could also imagine something more involved, like

instance.sync { instance.destroy }

versus

instance.async { instance.destroy }

but that's a pretty big syntax change.

To be clear, I think this should change, I'm just skeptical about the multiple method names. That said, I'm happy to be overruled.

[Temikus]

I think we can just reformat this as a positional parameter across the board.

Game plan for 2.0:

• [ ] Add async: named parameter to all .create and .destroy methods, i.e.: instance.destroy(async: false)
• [ ] Keep backwards compatibility during 2.0 but add deprecation warnings.
• [ ] Switch integration tests to use named parameters.
• [ ] Add unit tests to enforce behavior in new added models.

For 3.0:

• [ ] Remove all compatibility methods and switch to new behaviour.

The only thing I'm concerned about is if there are any weird corner cases with mixing positional and named parameters...

@icco WDYT?

[icco]

Seems like a solid plan @Temikus

[github-actions[bot]]

This issue has been marked inactive and will be closed if no further activity occurs.