Require callbacks to be void

[trowski]

Currently values returned by watcher callbacks are ignored. This PR specifies that Driver implementations should forward an implementation-specific exception to the loop error handler if something other than null is returned from a callback.

Returning from a callback is generally going to be an error, as this value is inaccessible. This is particularly relevant for callbacks returning a promise. If the promise fails, it is very likely that this failure will be silently ignored. Forwarding an exception to the error handler will allow such errors to be caught rather than go unnoticed.

[bwoebi]

If we pass #149 I do not consider this necessary as it then can be easily solved on the accessor level in the respective libraries.

[kelunik]

@bwoebi I think the concerns are the same then, so I think this is also valid with #149 merged.

[joshdifabio]

Returning from a callback is generally going to be an error

But not always. I don't see this as something this standard should be concerned with.

[kelunik]

@joshdifabio It is something to be concerned with, because it's an easy pitfall. While returning values directly might not be that important to check for, things like generators being accidentally used instead of wrapped in a function running it as coroutine, might be non-obvious.

[joshdifabio]

The fact that this is a SHOULD will result in bigger surprises than not having this restriction at all (I.e errors with some drivers but not with others). Do any existing loops error in these situations? What does node do?

[kelunik]

We should just convert it into a MUST. ;-)

[joshdifabio]

Then we can't create adapters for existing loops unless we wrap every callback in another which tests the return type of the original callback. Not exactly efficient. Unless this is something which most loops already do, I just don't see the need.

[trowski]

@joshdifabio This raises a good point. @kelunik the loop adaptor you just wrote would fall over unless you wrapped every callback just to ensure nothing was returned.

[kelunik]

@joshdifabio We have to wrap all callbacks for the React adapter anyway, see https://github.com/amphp/react-adapter/blob/ac2acacd7177f9cc69d786ce7eae308f1bc74b7d/lib/ReactAdapter.php

[trowski]

@kelunik Hah, so we do… I was thinking the tick callbacks weren't wrapped, but even those need the loop instance passed…

[joshdifabio]

@kelunik I actually meant an adapter which does the opposite; wraps a React loop, for example, so that it implements the interop standard. Perhaps that's not an important use case though.

[kelunik]

@joshdifabio An adapter the other way round is way more work and not worth the effort for us. People should just use the interop loop.

[kelunik]

@joshdifabio Are you fine with this PR now or do you still advise against it?

[joshdifabio]

I think that changing from SHOULD to MUST is an improvement, but I still don't like the change, because:

• Using a callback which returns or might return a non-null is not always an error (in fact, it will rarely be an error if we assume that developers know how the loop works). Therefore, I think this should simply be some kind of dev-mode feature within driver implementations which logs warnings if callbacks return a promise or generator
• Callbacks which might return a value must now always be wrapped (this is quite common within abstractions, e.g. even AsyncInterop\Promise::when() MAY return a value but generally won't) when being passed to the loop
• I don't think I've seen this functionality in existing loops, which suggests that it's unnecessary

I see far more drawbacks than benefits.

[kelunik]

I'd say it's not present in current loops, because (1) React just doesn't have the need (2) Amp currently runs Generators being returned as coroutines and auto-wraps them. This will no longer be the case with the interop loop, as both specifications will be decoupled. Forgetting Amp\wrap will probably be a common mistake that can happen, just not executing any code then, as the Generator isn't run.

I wouldn't include this PR in a world without Generators, but with them, the chance of accidental usage as direct callback seems to high.

[joshdifabio]

Using assertions to log warnings in such cases should be sufficient I think.

Edit: Do you think this mistake will be common simply because of Amp v1? If so, why burden this spec with that baggage given that the old Amp v1 reactor interface will be forgotten soon enough? If, rather, you're concerned that this is an easy mistake to make even without Amp v1, then I don't think this PR is a solution, because there are countless other places a developer could mistakenly pass a coroutine function which hasn't been wrapped, e.g. to Promise::when().

The more we talk about it, the more I think this is out of scope. Why are we even allowing coroutines to influence this spec, which sits at a lower level than coroutines do?

[kelunik]

@joshdifabio At that point it doesn't really matter whether it's an assertion or explicit exception. If we allow assertions like this, it's effectively banning return values for all callbacks.

[joshdifabio]

@joshdifabio At that point it doesn't really matter whether it's an assertion or explicit exception. If we allow assertions like this, it's effectively banning return values for all callbacks.

I'm talking about writing warnings somewhere, for example to a PSR logger, but leaving that to the implementation to decide. It could even be opt-in based on configuration of the driver in question. Either way I don't think it should be in this standard at all. Please have a look at the edit in my previous comment.

[kelunik]

Without anything in the specification, adding an assertion there is effectively against the spec then.

[joshdifabio]

Without anything in the specification, adding an assertion there is effectively against the spec then.

Just to be clear: I'm simply talking about using zero cost assertions to toggle logging of generators, etc. based on dev/production environment, I'm not talking about throwing using assert(). There is no change in behaviour, so I don't agree that it would go against the spec.

Either way, my primary point is that I don't believe that this restriction belongs in this spec, whether that restriction manifests itself in exceptions or in warnings written to a log. I edited an earlier post and I think perhaps you missed it, so I'll copy and paste it here:

Do you think this mistake will be common simply because of Amp v1? If so, why burden this spec with that baggage given that the old Amp v1 reactor interface will be forgotten soon enough? If, rather, you're concerned that this is an easy mistake to make even without Amp v1, then I don't think this PR is a solution, because there are countless other places a developer could mistakenly pass a coroutine function which hasn't been wrapped, e.g. to Promise::when().

The more we talk about it, the more I think this is out of scope. Why are we even allowing coroutines to influence this spec, which sits at a lower level than coroutines do?

[trowski]

I opened this PR because ignored coroutines and promises has been a practical problem experienced when writing code using this standard. However, I see @joshdifabio's point that this may be burdensome for others, inconsistent with other places callbacks are accepted, and something this standard should not be concerned with. The best fix is probably through documentation – if it seems like your coroutine isn't being run, first check to ensure you wrapped the callback. 😃