Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to select flushing strategy NoFlush is the default, but the `… #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tolysz
Copy link

@tolysz tolysz commented Dec 23, 2016

Allow to select flushing strategy NoFlush is the default, but the SyncFlush is useful with the incremental protocols eg. compression with websockets

…SyncFlush` is useful with the incremental compression eg. websockets
@hvr
Copy link
Member

hvr commented Dec 23, 2016

@tolysz unfortunately this is in conflict with the API design layed out in #6 which would provide us with exact control over flushing which is something that streaming frameworks require (and the reason some can't use zlib currently and have to resort to their own bindings)

@tolysz
Copy link
Author

tolysz commented Dec 23, 2016

;) I see no conflict. This change is independent of the other. I.e. it controls the default behavior. The other change (the proposed api change) will still be needed, as there is no explicit way flush buffer while building with NoFlush. In the real life, you need both as this library is useful in many possible protocols.

@hvr
Copy link
Member

hvr commented Dec 23, 2016

@tolysz the conflict I see (unless I'm wrongly assuming what this PR does):

the new API would give you the choice to either flush explicitly (via compressFlush) or don't flush (when using compressSupplyInput);

      CompressInputRequired {
         compressFlush       :: m (CompressStream m),
          compressSupplyInput :: S.ByteString -> m (CompressStream m)
      }

but if you change the default, compressFlush would serve no purpose anymore, wouldn't it?

PS: or let me ask differently (as then I may understand better): what does #10 make possible to do we couldn't already do if #6 was already implemented?

@tolysz
Copy link
Author

tolysz commented Dec 23, 2016

There will be applications which will require PUSH and PULL but you are right - not all combination for flush-strategy and explicit flushing will make sense.

The PR works and it should be transparent to most users. I am using it in implementation of permessage-deflate for WebSockets (formatRaw), thus I would need to call flush each time. PR will do it for me at the c level.
Flush flags are already implemented in zlib just are in not exposed module, this change simply makes use of them.
The alternative is to expose Streams from zlib and I could create zlib-flush library ;)

ps.
I do not change the default, I allow the default to be changed.

@dcoutts
Copy link
Contributor

dcoutts commented Feb 19, 2017

@tolysz are you sure this is what you really want? This would flush after every block, whereas even for websockets or whatever surely you'd want to flush at logical protocol boundaries rather than abitrary intermediate points depending on how the lazy bytestring was constructed.

I suspect that the stream interface with explicit flushing would work best for your use case, but I could be wrong. I'd be happy to get a more detailed explanation.

@emilypi
Copy link
Member

emilypi commented Feb 19, 2021

@tolysz is this still in flight? I believe @dcoutts makes some good points here.

@tolysz
Copy link
Author

tolysz commented Feb 19, 2021

It my still be useful, but in the end I switched to streaming-commons as I needed to implement it in the websockets library... and they already had it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants