Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Add two hooks to be executed when a chunk is downloaded. #159

Closed

Conversation

vincentbernat
Copy link

One hook is executed right before receiving a new chunk. The second
one is executed just after. parsertrace.c is updated to use those
hooks for display.

I am using this for experimentation with rendering. This is more reliable than looking at timestamps to know where chunks start and end.

@bnoordhuis
Copy link
Member

LGTM but some regression tests would be nice. I confess I don't understand your comment about looking at timestamps.

@vincentbernat
Copy link
Author

I have updated the patch. Tests are failing with "fast" settings. I need to investigate a bit.

One hook is executed right before receiving a new chunk. The second
one is executed just after. parsertrace.c is updated to use those
hooks for display.
@vincentbernat
Copy link
Author

I have updated the patch. I had some difficulties to understand the difference between CALLBACK_NOTIFY and CALLBACK_NOTIFY_NOADVANCE and how this was important when the parser was paused in a callback. I have been mislead by the fact that the tests failed only in "fast" mode.

In test_message_pause(), shouldn't we replace:

      if (HTTP_PARSER_ERRNO(parser) == HPE_STRICT) {
        parser_free();
        return;
      }

by:

assert (HTTP_PARSER_ERRNO(parser) != HPE_STRICT)

Otherwise, when we get a failed strict-mode check, we just abort the whole test! I am opening a pull request for that.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

@vincentbernat ... is this still something you'd like to pursue? If so, the PR would need to be rebased and updated /cc @bnoordhuis @indutny

@vincentbernat
Copy link
Author

I could update the PR, but would it get more feedback today than it has two years ago? Same for #161 and #162. I understand there is no interest to merge and as I don't need those changes anymore, I don't want to spend more time on them.

@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

@vincentbernat ... yeah, it would get more feedback as there are a lot more people looking now (That's no guarantee that it would land tho)

@derekargueta
Copy link
Contributor

Can this PR be closed since we have on_chunk_header and on_chunk_complete today?

@bnoordhuis
Copy link
Member

Thanks, it looks like this can indeed be closed.

@bnoordhuis bnoordhuis closed this May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants