Ensure that we have timeouts in place when reading/writing nodes in IPFS

[aphelionz]

From: https://github.com/ipfs/dynamic-data-and-capabilities/issues/50

Ensure that we have timeouts in place when reading/writing nodes in IPFS

[aphelionz]

cc @satazor

[satazor]

@aphelionz Should I take care of this one or wait for #213 to be merged? I'm asking because there will be some conflicts.

[aphelionz]

Sure, go ahead

[satazor]

Before I actually implement this, I will need some feedback. I will use the p-timeout module but consumers of ipfs-log need to be in control of the actual timeout in ms. The default should be "no timeout" to keep things unchanged but what's the best way to pass the actual timeout?

A Log can be instantiated directly via its constructor or via static functions, such as fromMultihash. This means that the static functions already must include the ability to specify the read timeout because they perform I/O. The most practical way is to add a timeout argument to all of these functions but I'm worried about the number of arguments. As an example:

static async fromMultihash (ipfs, access, identity, hash, length = -1, exclude, onProgressCallback) {

already has 7 arguments, 3 of them being optional. I think that the most "javascripty" way to do this would be:

static async fromMultihash (ipfs, access, identity, hash, options) {

where options would look like:

{
  length,
  exclude,
  onProgress,
}

This way, we could add more optional arguments, like the timeout argument I'm proposing.

Another solution is to configure the read & write timeouts statically, but it feels clunky.

---

I would love to hear more about this and how to proceed.

[aphelionz]

@satazor Let's keep it as a discrete argument for now, even though there's a lot.

We want to favor a transparent function signature that doesn't "hide" the arguments in an options object. Even with the new syntax that exposes the object members, it it's still hard to tell what options are available. At least with the large number of discrete arguments, it's easy for the user to tell what they are, and what they expect.

The reminds me, I need to add the sortFn argument to those functions too!

[satazor]

I understand your concerns but currently it’s very cumbursome to specify a value to an optional argument, especially if it’s the last one: one has specify undefined for all the arguments that want to keep the default.

Of course that all this topic is subjective and boils down to personal preference, but my personal view is that in a language that doesn’t support method overloading, something like my proposal is better than what we have now. I tend to prefer ergonomy vs “fully transparent” functions.

Nevertheless, I will advance with adding the argument to the end of the list.

[aphelionz]

I hear you, and I think with the new destructured argument syntax we might be able to find a middle ground in the near future. In the meantime, thanks for being flexible on this.

[haadcode]

Not sure if does exactly what you need @satazor, but we do have per-entry-load/fetch timeout in https://github.com/orbitdb/ipfs-log/blob/master/src/entry-io.js#L65.

[shamb0t]

I agree that the signature is long and it'd be preferable to shrink it. We were having a similar issue in orbit-db and found that having required arguments as discrete and anything with a default value could be passed in the options worked well

[aphelionz]

This is the best example I could find for the destructured function parameters, essentially using the new destructured assignment notation to simulate named parameters. I think we may be able to achieve the best of both worlds with this approach. http://2ality.com/2015/01/es6-destructuring.html#simulating-named-parameters-in-javascript

This could potentially allow us to have an optional options parameter of options ;) but also keep some semblance of transparency in the function signatures.

[satazor]

@aphelionz Yep, by using destructuring, the "arguments" are pretty visible which addresses your concerns. Neverthless, I shall proceed with adding another argument in the PR and later we discuss refactor the optional arguments into options right?

[satazor]

As @haadcode pointed out, we already have a timeout argument in Entry.fetchAll & Entry.fetchParallel. In those two functions, the timeout argument comes before the onProgressCallback and it should be like this everywhere to maintain consistency. By everywhere, I mean: LogIO.fromEntryCid, LogIO.fromJSON, LogIO.fromEntry, Log.fromEntryCid, Log.fromJSON, Log.fromEntry. The problem is that if I add it before onProgressCallback, it will be a breaking change for the functions that are part of the public API. Possible solutions:

1. Break consistency and put timeout after the onProgressCallback on LogIO.fromEntryCid, LogIO.fromJSON, LogIO.fromEntry, Log.fromEntryCid, Log.fromJSON, Log.fromEntry
2. Move timeout after onProgressCallback on Entry.fetchAll & Entry.fetchParallel; we can do this because it's not part of the public API
3. Place timeout before onProgressCallback but do a typeof check in case timeout is a function and shift arguments, for backwards compatibility
4. Adopt the options strategy I mentioned above in a backwards compatible manner. The options object doesn't suffer from the positional issues I'm describing here. We could create a util/helper that would ensure the backwards compatibility and would be reused in a lot of places.

Let me know what your thoughts are @aphelionz @haadcode @shamb0t

[shamb0t]

@satazor @aphelionz I think adopting the options strategy will work best moving forward, with the required args being discrete and any optional values passed in options. However given that necessary changes may touch many places I would also support putting timeout after onProgressCallback for now and move the optional args to options in a subsequent PR. How does that sound?

[aphelionz]

@satazor @shamb0t sure, I can address this in my work in https://github.com/orbitdb/ipfs-log/issues/219. Go ahead and put timeout after onProgressCallback to get this resolved.

[satazor]

PR at #219 but I don't really like the looks of it because the Log.fromJSON already exposed the timeout before onProgressCallback. This means that the public API is inconsistent. Should we wait for #219 that brings the options?

Moreover, there's a concurrency argument that we should expose as an option too.

[aphelionz]

That makes sense to me @satazor

[aphelionz]

@satazor My PR #220 has been merged. You should be able to work your timeouts into the destructure options object now.

[aphelionz]

Removing my assignment as I'm not currently working on this.

[shamb0t]

Resolved by #258