Souler commented 5 years ago

Node.js introduced the destroy method for streams, which is missing from the current RecordStream implementation

We ran into this issue when attempting to prematurely end a clickhouse RecordStream generated by a querying a large dataset. The app process "hanged" until force closed or the query stream finished pushing data.

Some reference links: https://nodejs.org/docs/latest-v8.x/api/stream.html#stream_readable_destroy_error https://medium.com/@CWMma/whats-new-for-streams-in-node-8-736d431083df

codecov-io commented 5 years ago

Codecov Report

Merging #23 into master will increase coverage by 0.1%. The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #23     +/-   ##
=========================================
+ Coverage   93.58%   93.68%   +0.1%     
=========================================
  Files           5        5             
  Lines         296      301      +5     
=========================================
+ Hits          277      282      +5     
  Misses         19       19

Impacted Files	Coverage Δ
src/streams.js	`98.76% <100%> (+0.08%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dbe6a2a...004b022. Read the comment docs.

nezed commented 5 years ago

Looks nice! But i think there must be some tests

Souler commented 5 years ago

Yes, I'm already working on them. Sorry for the incomplete PR

Should be done by now

apla / node-clickhouse

Add destroy to RecordStream #23

Codecov Report