Add handlers for when the rewriting is finished. #27

gabi-250 · 2019-11-11T18:27:13Z

This should enable users to add a handler that is called after the rewriter finishes.

src/transform_stream/dispatcher.rs

inikulin · 2019-11-11T20:22:30Z

examples/mixed_content_rewriter/main.rs

@@ -37,6 +37,10 @@ fn main() {
                    }
                ),
            ],
+            end_handlers: vec![end!(|| {


This can probably a part of document_content_handlers as it's basically a handler for the end of the document content.

inikulin · 2019-11-11T20:27:01Z

src/rewriter/settings.rs

@@ -8,6 +8,7 @@ pub type CommentHandler<'h> = Box<dyn FnMut(&mut Comment) -> HandlerResult + 'h>
 pub type TextHandler<'h> = Box<dyn FnMut(&mut TextChunk) -> HandlerResult + 'h>;
 pub type ElementHandler<'h> = Box<dyn FnMut(&mut Element) -> HandlerResult + 'h>;
 pub type EndTagHandler<'h> = Box<dyn FnOnce(&mut EndTag) -> HandlerResult + 'h>;
+pub type EndHandler<'h> = Box<dyn FnMut() -> HandlerResult + 'h>;


It can be FnOnce as it will be called only once in the end. Also, note that we'd like to have an ability to emit content in the end of the document as well. We'd like to follow the same API as for other rewritable units: it should be possible to emit content by multiple function calls with different types of content. So, we need to provide DocumentEnd rewritable unit which exposes just a single method append.

gabi-250 · 2019-11-15T15:13:29Z

@inikulin I think I've addressed your comments. I added an example that uses the handler (examples/on_end_append). I've had to make content_to_bytes pub(crate), because I used it in the DocumentEnd rewritable unit to implement append:

https://github.com/cloudflare/cool-thing/blob/cb5f31a1b7eb98e4e34c1540df614e5d9e5fef82/src/rewritable_units/document_end.rs#L21-L24

gabi-250 · 2019-11-15T15:16:47Z

Once this is reviewed, I am happy to update the tests and documentation where applicable. Also, @xtuc suggested implementing an error handler (which would be needed in order for cool-thing to fully support my usecase). If you want to support error handlers, I can implement that as part of this PR as well.

inikulin · 2019-11-18T12:46:16Z

@gabi-250 We have everything in place for the error handler in cool-thing already. We need to implement in workers code only.

inikulin

We also need C API implementation and tests

inikulin · 2019-11-18T12:49:00Z

examples/on_end_append/main.rs

+    };
+
+    // Create the rewriter
+    let mut rewriter = HtmlRewriter::try_new(


Thanks for the example. However, we try to use examples directory to show how to implement some real world scenarios. So, it's worth changing it to do something more useful, or just remove it.

inikulin · 2019-11-18T12:50:40Z

src/rewritable_units/mutations.rs

@@ -13,7 +13,7 @@ pub enum ContentType {
 }

 #[inline]
-fn content_to_bytes(
+pub(crate) fn content_to_bytes(


It can be pub(super)

inikulin · 2019-11-18T12:53:23Z

src/transform_stream/dispatcher.rs

+
+        self.transform_controller
+            .handle_end(&mut document_end)
+            .expect("finish failed");


This shouldn't panic. We need to propagate error to the rewriter's end() return result

inikulin · 2019-11-18T12:58:10Z

src/rewritable_units/document_end.rs

+
+    #[inline]
+    pub fn append(&mut self, content: &str, content_type: ContentType) {
+        content_to_bytes(content, content_type, self.encoding, self.output_handler);


This is inconsistent with other append functions - here it appends content to the previously inserted content, whereas, in all other cases, consequent calls to append insert content right after the rewritable unit.

Therefore, I suggest to not leak output sink in the rewritable unit and just collect appended content, implement Serialize on the unit and use it in the dispatcher.

I think subsequent calls are meant to append to the previously inserted content (like the append of Element), so I think it should be fine to append the content on the fly using content_to_bytes.

I could implement Serialize for DocumentEnd, but I would be duplicating the implementation of content_to_bytes.

inikulin · 2019-11-18T12:58:32Z

src/rewritable_units/document_end.rs

+    encoding: &'static Encoding,
+}
+
+impl<'a> DocumentEnd<'a> {


Documentation is required

xtuc · 2019-11-18T14:42:55Z

src/rewriter/settings.rs

@@ -299,6 +308,13 @@ macro_rules! doc_comments {
    };
 }

+#[macro_export(local_inner_macros)]
+macro_rules! end {


Is the macro used?

It's supposed to be used by the users of the API (like in the example I removed in eedcc36).

inikulin · 2019-11-18T14:46:46Z

src/transform_stream/dispatcher.rs

-        self.transform_controller
-            .handle_end(&mut document_end)
-            .expect("finish failed");
+        let result = self.transform_controller.handle_end(&mut document_end);


We should propagate the error immediately and don't perform any further actions

inikulin · 2019-11-18T14:58:59Z

src/transform_stream/dispatcher.rs

        self.flush_remaining_input(input, input.len());

+        let output_sink = &mut self.output_sink;
+        let mut output_sink_handle_chunk = |c: &[u8]| output_sink.handle_chunk(c);


We can just store mutable reference for the output_sink in DocumentEnd, so we won't need intermediate closure

inikulin · 2019-11-19T17:20:16Z

@gabi-250 Rust part lgtm, thanks. Now we need to test it properly and add C API for it.

gabi-250 · 2019-11-20T14:20:18Z

Ready for re-review.

I added tests for the document end handler in src/rewritable_units/document_end.rs and in src/rewriter/mod.rs

I added the cool_thing_doc_end_append function to the C API, as well as a couple of tests for it.

I've had to make a slight change to the impl_replace_byte macro, because on some inputs, the output sink would receive an empty chunk before the end of the input stream:
https://github.com/cloudflare/cool-thing/blob/857da3a4f021d0f2f53cf6e8f5761c1e9bf0ce87/src/base/bytes.rs#L80-L84

inikulin · 2019-11-20T16:53:48Z

src/base/bytes.rs

-                    $output_handler(&tail[..pos]);
+                    // If pos is 0, tail[..pos] will be empty, thus erroneously interpreted
+                    // as a finalizing chunk.
+                    if pos != 0 {


Nice find! We can refactor it further:

let chunk = &tail[..pos]; if !chunk.is_empty() { $output_handler(chunk); }

This way the code will be self descriptive as we obviously don't need to invoke the handler for empty chunks.

And you won't need the comment.

Also, it would be nice to have a regression test for this as this is obviously a bug that can cause abrupt responses in workers.

Passing an empty chunk this way doesn't actually cause an error (except in the tests), since end is not called twice. The only reason I spotted this is because the test output sink checks if an empty chunk is received twice (it caused one of my tests to fail):
https://github.com/cloudflare/cool-thing/blob/ab462cbad4e6ac05d0c00a74f5a562eb6beae20c/src/lib.rs#L118-L128

Regardless, I agree it's a bug. I'll write a regression test.

inikulin · 2019-11-20T16:56:12Z

src/rewriter/mod.rs

@@ -529,6 +529,67 @@ mod tests {
        }
    }

+    #[test]
+    fn append_at_the_end() {


This can be moved to document_end.rs, as tests here are for general cases, not specific to a particular rewritable unit.

I've removed these altogether, since the tests in document_end.rs are almost exactly the same.

inikulin · 2019-11-20T16:57:21Z

c-api/include/cool_thing.h

@@ -150,7 +155,8 @@ void cool_thing_rewriter_builder_add_document_content_handlers(
    cool_thing_comment_handler_t comment_handler,
    void *comment_handler_user_data,
    cool_thing_text_handler_handler_t text_handler,
-    void *text_handler_user_data
+    void *text_handler_user_data,
+    cool_thing_doc_end_handler_t doc_end_handler


We need user data for this handler as well

inikulin · 2019-11-20T16:57:51Z

c-api/include/cool_thing.h

@@ -651,6 +657,13 @@ void cool_thing_element_user_data_set(
 // Returns user data attached to the text chunk.
 void *cool_thing_element_user_data_get(const cool_thing_element_t *element);

+int cool_thing_doc_end_append(


inikulin

Nicely done. Thanks!

inikulin · 2019-11-26T15:12:33Z

@gabi-250 this requires lol-rebase now

* Make the end handler a document content handler. * Allow appending at the end of the document. * Append chunks on the fly. * Remove unneeded example. * Propagate error to `end()`. * Propagate the error immediately. * Store a reference to the OutputSink in DocumentEnd.

gabi-250 · 2019-11-26T17:10:15Z

@inikulin done

gabi-250 requested a review from inikulin November 11, 2019 18:27

gabi-250 requested a review from a team as a code owner November 11, 2019 18:27

gabi-250 changed the title ~~Allow handlers for the end of parsing.~~ Add handlers for when the rewriting is finished. Nov 11, 2019

xtuc reviewed Nov 11, 2019

View reviewed changes

src/transform_stream/dispatcher.rs Outdated Show resolved Hide resolved

inikulin reviewed Nov 11, 2019

View reviewed changes

sejoker self-requested a review November 13, 2019 16:42

gabi-250 force-pushed the add-end-handlers branch 3 times, most recently from a63116a to cb5f31a Compare November 15, 2019 15:06

gabi-250 requested review from xtuc and inikulin November 15, 2019 15:20

inikulin requested changes Nov 18, 2019

View reviewed changes

xtuc reviewed Nov 18, 2019

View reviewed changes

inikulin requested changes Nov 18, 2019

View reviewed changes

gabi-250 force-pushed the add-end-handlers branch 3 times, most recently from 919febd to 80494cb Compare November 19, 2019 16:37

gabi-250 force-pushed the add-end-handlers branch 3 times, most recently from 57aab30 to 8626f65 Compare November 20, 2019 13:46

gabi-250 requested review from inikulin and xtuc November 20, 2019 14:20

inikulin requested changes Nov 20, 2019

View reviewed changes

gabi-250 requested a review from inikulin November 21, 2019 09:22

inikulin approved these changes Nov 21, 2019

View reviewed changes

gabi-250 force-pushed the add-end-handlers branch 4 times, most recently from 86a2b70 to 428db19 Compare November 26, 2019 16:36

gabi-250 added 4 commits November 26, 2019 16:43

Test if appending at the end works.

f1931c1

Extend the C API to support document end handlers.

721f51d

Refactor condition.

7d2d346

gabi-250 force-pushed the add-end-handlers branch from 428db19 to 202d8b6 Compare November 26, 2019 16:43

gabi-250 added 3 commits November 26, 2019 16:54

Pass user data to the document end handler.

5c48151

Document the C API function.

a172e50

Add regression test.

e3160ce

gabi-250 force-pushed the add-end-handlers branch from 202d8b6 to e3160ce Compare November 26, 2019 16:55

inikulin merged commit 1aedf36 into master Nov 26, 2019

inikulin deleted the add-end-handlers branch November 26, 2019 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handlers for when the rewriting is finished. #27

Add handlers for when the rewriting is finished. #27

gabi-250 commented Nov 11, 2019 •

edited

inikulin Nov 11, 2019

inikulin Nov 11, 2019

gabi-250 commented Nov 15, 2019

gabi-250 commented Nov 15, 2019

inikulin commented Nov 18, 2019

inikulin left a comment

inikulin Nov 18, 2019

inikulin Nov 18, 2019

inikulin Nov 18, 2019

inikulin Nov 18, 2019

gabi-250 Nov 18, 2019

inikulin Nov 18, 2019

xtuc Nov 18, 2019

gabi-250 Nov 18, 2019

inikulin Nov 18, 2019

inikulin Nov 18, 2019

inikulin commented Nov 19, 2019

gabi-250 commented Nov 20, 2019

inikulin Nov 20, 2019

inikulin Nov 20, 2019

inikulin Nov 20, 2019

gabi-250 Nov 20, 2019 •

edited

inikulin Nov 20, 2019

gabi-250 Nov 20, 2019

inikulin Nov 20, 2019

gabi-250 Nov 20, 2019

inikulin Nov 20, 2019

gabi-250 Nov 20, 2019

inikulin left a comment

inikulin commented Nov 26, 2019

gabi-250 commented Nov 26, 2019

Add handlers for when the rewriting is finished. #27

Add handlers for when the rewriting is finished. #27

Conversation

gabi-250 commented Nov 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-250 commented Nov 15, 2019

gabi-250 commented Nov 15, 2019

inikulin commented Nov 18, 2019

inikulin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inikulin commented Nov 19, 2019

gabi-250 commented Nov 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-250 Nov 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inikulin left a comment

Choose a reason for hiding this comment

inikulin commented Nov 26, 2019

gabi-250 commented Nov 26, 2019

gabi-250 commented Nov 11, 2019 •

edited

gabi-250 Nov 20, 2019 •

edited