search

poelstra / ts-stream

Type-safe object streams with seamless support for backpressure, ending, and error handling
MIT License
65 stars 13 forks source link

I hit a bug in batch(), assertin browser is a pain (dropping it) #57

Closed MarcWeber closed 3 years ago

MarcWeber commented 3 years ago

I provided an alternative maybe simple implementation: https://github.com/MarcWeber/ts-stream

I don't know whether I understood the correct desired behavior of the reference implementation which is why I just added mine.

i tried triggering the case in the test suite but somehow failed. Looking at the code above it looks like setTimeout's flush doesn't reset pendingWrite or such. The issue I had is that batch halted the stream.

I also didn't get why track() is helpful at all cause the implementation I provide doesn't require it. track's implementation eventually also would be easier having just a 'state' like flag assigned a string of type "pending" "fullfilled" "rejected".

Also mind that in track() there might be a race condition if the promise gets passed in resolved state then the defaults for isFullfilled etc might get overwritten.

poelstra commented 3 years ago

I would expect assert to be able to be 'filled in' by a bundler, just like other NodeJS builtins typically can, but I can also perfectly live with replacing it with a more standard throw :)

As with your other issue, I'm not entirely sure what's happening, or what you're trying to do. But do note that the batch function was pretty heavily discussed and tested to be generically useable, and as such should be safe to use.

If you found a bug, I'd be happy to see a reproduction scenario of course, preferably by adding a test to its existing set of test cases.

Also note that it's perfectly fine to create your own transforms for your own specific use-cases. It's very likely that a specific solution for your use-case can indeed be way simpler than the completely generic one, so feel free to use that. Also note that you should not need to add it to ts-stream itself, you can just add it to your own project, and still use it with ts-stream. I'm not inclined to add a non-generic version to the library, btw.

MarcWeber commented 3 years ago

I still had some small bugs in my code (fixed).

https://mawercer.de/tmp/tmp/ts-stream-test-case.patch Adding my version to test suite shows that it doesn't pass batcher tests.

Get this patch: https://mawercer.de/tmp/tmp/test-case.patch Expected: that [6] [7] but got [5,6,7] or such. That's why I failed at adding a testcase cause it looks like I fail to understand current implementation or there is a bug. If the delay is 10ms and if batchTimeout is 2 each item should be passed as a single item.

poelstra commented 3 years ago 
Get this patch: https://mawercer.de/tmp/tmp/test-case.patch
Expected: that [6] [7] but got [5,6,7] or such.
That's why I failed at adding a testcase cause it looks like I fail to understand current implementation or there is a bug. If the delay is 10ms and if batchTimeout is 2 each item should be passed as a single item.

Yup, there's a bug. If there was timeout-flushed write before, it appears it won't flush the next timed out write after that. I've pushed a fix for it, and removed the assert, and fixed that track issue. (Version still needs to be published to NPM.)

Note that the track() issue shouldn't ever happen if the used promise implementation is well-behaved (because they should never synchronously call their then callbacks, not even when the promise is already resolved), but it's good to be robust of course.

Thanks for reporting, and providing a clear reproduction scenario!

poelstra commented 3 years ago

Published to npm as 3.0.0.