GNU Radio code can block reactor

[bitglue]

As noticed here: https://github.com/kpreid/shinysdr/pull/82#issuecomment-306489971

Demodulators are instantiated within the reactor thread. Since demodulators are also GNU Radio blocks, this involves calling GNU Radio code which isn't designed with Twisted's event model in mind. Sometimes there will be blocking code in creating a ... block :grimacing: For a specific example, the WAV file sink opens a file (and maybe also writes the WAV header?)

Maybe these actions can be run through deferToThread?

[kpreid]

This particular problem is a very low-priority issue. Even demodulators that are cheap to create will cause a hiccup when the they are inserted in the flow graph, because reconfiguration stops the entire flow graph temporarily. GNU Radio is not currently designed to do what we're doing smoothly.

[bitglue]

My bigger concern is what happens when it does become an issue, and for some reason there's a block which when created blocks for more than a "hiccup"? Even if there's no such case now (except an NFS mount wherever the WSPR demodulator puts its files), there's no expectation for GNU Radio code to not block.

It's possible, perhaps even likely such a case will be encountered, and with the current API there's nothing that can be done about it.

My suggestion is to add a .get_block() to IDemodulator which can optionally return a deferred. A return self can trivially be added to existing implementations (or even do that implicitly, with a deprecation warning), while deferToThread() is now an option for the future. I think it will be easier to do this now, since the number of people expecting a stable API is very small.

I'm not expecting you to do anything immediately, just acknowledge this is an acceptable thing. No guarantees, but I might attempt that effort.

[kpreid]

My suggestion is to add a .get_block() to IDemodulator which can optionally return a deferred.

This solution does not generalize to all possible blocking. For example, imagine a demodulator which, in order to respond correctly to get_band_shape or get_output_type, requires starting a subprocess and asking it a question.

Therefore, I would be inclined to, instead, arrange so that the IDemodulator instance itself is returned via some Deferred. Thus the plugin code can do whatever initialization it needs at its own speed.

---

Note that there is a closely related problem around Devices. Note the following facts about the current system:

• The only way to recover from lost communication with a device is to restart the server.
• OsmoSDRDevice has to jump through hoops to implement T/R switching.
• Using a soundcard + hamlib device requires you to fiddle with Deferreds in the config file.
• If hardware is missing the server cannot start because the device fails in construction.

I have previously considered these consequences of a single architecture mistake: it should be possible to create a Device without actually connecting to the underlying hardware, and it should be possible to generically ask a device to disconnect and reconnect (which could then more easily be asynchronous).

[bitglue]

This solution does not generalize to all possible blocking. For example, imagine a demodulator which, in order to respond correctly to get_band_shape or get_output_type, requires starting a subprocess and asking it a question.

Obtaining the demodulator though a deferred doesn't generalize to all possible blocking, either. It only generalizes to the kinds of blocking that might happen on initialization.

The places you get suck wishing "I wish this could return a deferred" are usually methods starting with "get".

"What's your filter shape?"

"I don't know yet."

So what if there are no such methods? Just do nothing at all until the relevant information is available. So, set_band_shape() and set_output_type(), as part of the interface to something the modulator knows about. Maybe the context object?

You'll end up doing this anyway, if a method can return a deferred. When you get that deferred back from get_band_shape(), you'll just immediately .addCallback(self.set_band_shape()). So why not just skip the complexity of the deferred?

Thinking about the Twisted code itself, this seems to be the more common pattern. For example, you don't call getData() and get a deferred; you implement Protocol.dataReceived(). Deferreds are really around just for interactions which are too simple or ephemeral to merit a more detailed interface, like deferToThread().

The interaction with a demodulator does not seem simple or ephemeral (especially since they aren't strictly demodulators, but mode plugins), so are deferreds really the most appropriate tool?

[bitglue]

I have previously considered these consequences of a single architecture mistake: it should be possible to create a Device without actually connecting to the underlying hardware

You mean a side-effect in __init__? :smile:

[kpreid]

create a Device without actually connecting to the underlying hardware

You mean a side-effect in __init__?

Touché.

But I would draw a distinction between “side effects in __init__ versus other code locations” and “a device can be closed and (re)opened multiple times”. Following the former rule does not automatically get you the latter.

I also do not think that a rule of avoiding side effects in __init__ is a good general principle in Python. (In languages where calls to constructor are special, there are reasons to wrap nontrivial constructors in factory methods, yes.) Insofar as it is a good rule it is just a specialization of not making any function do an indivisible block of work you might want to subdivide.

Obtaining the demodulator though a deferred doesn't generalize to all possible blocking, either. It only generalizes to the kinds of blocking that might happen on initialization.

True, but once a demodulator exists, insofar as it changes over time, it can make those changes on its own schedule.

So, set_band_shape() and set_output_type(), as part of the interface to something the modulator knows about. Maybe the context object?

This is an admirable design in another way: it removes any way for the demodulator to violate its interface by returning different answers at different times, because the context can always reject the set operation. However, I currently think it is valuable that demodulators can be objects written in the style of straightforward Python+GR code. This would move away from that into something weirder.

It might be a good idea anyway. Filed for future consideration.

Thinking about the Twisted code itself, this seems to be the more common pattern. For example, you don't call getData() and get a deferred; you implement Protocol.dataReceived().

I don't think this is a good example: the effect of receiving new data (whether that is done in a push or pull style) is much different from a getter.

[bitglue]

It occurs to me adjustable filter widths might be especially elegant with a set rather than get pattern. Maybe you have something like this:

class IFilter(Interface):
    markers = Attribute("Locations to draw cursors. Typically the carrier frequency, but in the case of RTTY for example maybe separate mark and space markers.")
    range = Attribute("tuple of (low, high) bounds to draw for the filter")
    can_set_range = Attribute()

    def set_range( (low, high) ):
        pass

A demodulator can then self.context.set_filter(my_implementation_of_ifilter), and provide all at once the information necessary to draw it, and the callbacks necessary to respond to user actions to change it, if supported. (Perhaps even allowing multiple filters to be set for PSK31, depending on how the UI for that works out.)

These could just be defined as callbacks on IDemodulator of course, but when you consider all the different kinds of things which may or may not be applicable to a particular mode, I could see that interface getting very large with most of it irrelevant to a particular implementation.

[bitglue]

Again, want to reiterate I'm just looking for some acknowledgement that these changes would be acceptable. Then I might implement them. I don't want to spend days on a refactor that's going to be rejected.

[kpreid]

Unfortunately, the answer that I have right now is “no”. Not because moving in this direction in any way is unacceptable, but because there are intentional architectural constraints that aren't actually well-documented, and I think that right now a refactor by anyone but me would be a mess of “no, not that way” and redoing lots of work.

However, I do appreciate your constructive criticism and pointing out things I haven't thought of and it will direct my plans for (near-)future work.