uNetworking / uWebSockets.js

μWebSockets for Node.js back-ends :metal:
Apache License 2.0
7.76k stars 563 forks source link

Better onWritable documentation, and more user friendly warning on ill-use. #915

Open uNetworkingAB opened 1 year ago

uNetworkingAB commented 1 year ago

There is a mistake in the stricness of onWritable return checks. It really only checks if onWritable returns empty or non-empty, while the real check should be if it returns a boolean or or not. The return value of onWritable must be nothing but a boolean.

This check should also have a nice, clear error message like the others we have for the 2 other checks.

uNetworkingAB commented 1 year ago

Ah so the problem is that an async function returns Promise, which is truthy. So this gets interpreted as true:

Writing nothing is always success, so by default you must return true.

So this is actually correct, as writing nothing should return true.

Only if you write inside onWritable, and fail, should you return false. So for async functions it works correctly already

uNetworkingAB commented 1 year ago

Alright I remember now.

Returning false in onWritable is an optimization where the lib won't perform an extra attempt at sending, since false means the user just sent something themselves and failed. This means that all other returns than false, will try and drain the backpressure. This whole feature should be better documented to explain why onWritable wants boolean.

We should accept Promise, Boolean and only warn on empty return. Currently it errors with an ugly message on empty return.