Suggestion: Rename HttpProcessCallback2 or HttpProcessCallback

[moigagoo]

There are to types for process callbacks:

• HttpProcessCallback: https://github.com/status-im/nim-chronos/blob/master/chronos/apps/http/httpserver.nim#L67
• HttpProcessCallback2: https://github.com/status-im/nim-chronos/blob/master/chronos/apps/http/httpserver.nim#L71

The first one is not async, the second one is.

This is confusing for two reasons:

1. Chronos is an async runtime, so you generally expect things to be async it it. The fact that HttpProcessCallback is not annotated with async is surprising.
2. The names of the types do not tell anything about their "asyncness" or lack thereof. I would thing these are an older and a newer version but there is no deprecation notice so they both must be valid and usable.

I suggest we rename either or both of those types, e.g.:

• HttpProcessCallback → HttpProcessCallbackSync (since async the default)
• HttpProcessCallback2 → HttpProcessCallback (no confusing 2 in the name)

[arnetheduck]

These are both async (ie anything returning a Future is async implicitly) - they differ in which exceptions the callback is allowed to raise - 2 was added with more strict requirements while the old callback remains for backwards compatibility.

[moigagoo]

Thanks for the info.

Shouldn't HttpProcessCallback be annotated with {.deprecated.} if it exists only for compat?

[cheatfate]

I think you can't annotate argument as being deprecated, but probably constructors which are using it could be marked as deprecated.

[moigagoo]

I think you can't annotate argument as being deprecated, but probably constructors which are using it could be marked as deprecated.

I mean annotate the type as deprecated:

  HttpProcessCallback* {.deprecated: "Use HttpProcessCallback2 instead.".} =
    proc(req: RequestFence): Future[HttpResponseRef] {.
      gcsafe, raises: [].}

[arnetheduck]

Shouldn't HttpProcessCallback be annotated with {.deprecated.} if it exists only for compat?

"backwards" was the wrong word - it's necessary for "async" compat without raises information which remains a "supported" way of using chronos (ie we do not enforce raises).

[moigagoo]

"backwards" was the wrong word - it's necessary for "async" compat without raises information which remains a "supported" way of using chronos (ie we do not enforce raises).

Thanks for the clarification.

Those types would definitely benefit from more direct naming, e.g.:

• HttpProcessCallback2 → HttpProcessCallbackCancellable, HttpProcessCallbackRaising, or simply HttpProcessCallback (since things are cancellable by default)
• HttpProcessCallback → HttpProcessCallbackNonCancellable or HttpProcessCallbackNonRaising

Not sure if this is a change that is worth making though. Descriptive names are better but this would require updating depending packages.

[cheatfate]

Both callbacks are cancellable.

[moigagoo]

Cancellable was a wrong word. How do you call a callback that raises CancelledError as opposed to one that raises nothing? Cancelling?

My point is that the current names don't mean anything in particular and are therefore confusing when you first encounter them (at least I was). There's legitimate difference in behavior that's not reflected in the naming.

I don't think it's major issue, more of an inconvenience. But it is an inconvenience :-)