Skip to content

Runtime: flush output buffer before accessing offset.#778

Closed
corwin-of-amber wants to merge 1 commit into
ocsigen:masterfrom
corwin-of-amber:master
Closed

Runtime: flush output buffer before accessing offset.#778
corwin-of-amber wants to merge 1 commit into
ocsigen:masterfrom
corwin-of-amber:master

Conversation

@corwin-of-amber
Copy link
Copy Markdown
Contributor

The simplest solution to #777 would be to flush the buffer first and ask questions later.

I suspect that any performance degradation resulting from this would be unnoticeable.

Comment thread runtime/io.js Outdated
@@ -396,24 +396,30 @@ function caml_output_value (chanid,v,_flags) {
//Provides: caml_ml_seek_out
//Requires: caml_ml_channels
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the //Requires: bits ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added them. Sorry. 🤕

@TyOverby
Copy link
Copy Markdown
Collaborator

TyOverby commented Apr 25, 2019

@corwin-of-amber, do you have a repro of this bug that could be added as a test? I've recently expanded the capabilities of unit testing in the compiler and runtime, and this seems like a great scenario to see if I've covered some common use-cases.

For examples, see the /compiler/tests/ directory.

@hhugo
Copy link
Copy Markdown
Member

hhugo commented Apr 25, 2019

I've tried to come up with a test without success #779

@corwin-of-amber
Copy link
Copy Markdown
Contributor Author

I've added the test in #777.

@hhugo
Copy link
Copy Markdown
Member

hhugo commented Apr 26, 2019

Thanks a lot. I've merged your fixes together with some tests. I'm closing this one.

@hhugo hhugo closed this Apr 26, 2019
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.

3 participants