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

WIP chore(log): migrate std/log to use the Web Streams API #3969

Closed
wants to merge 14 commits into from

Conversation

syhol
Copy link
Contributor

@syhol syhol commented Dec 15, 2023

Tackling this issue: #3907

Whats left to do in this PR:

  • Tests need updating
  • Race conditions need handling and testing
  • Figure out how destroy is handled by callers now that its async
  • Understand how to "unload" works with promises and if we need to do something about the destroy handler
    • It does not wait for promises to resolve, this is potentially a big problem as this means we will always lose the last write when the flush is async.
  • Should new properties use protected or #hashName
  • Delete PR specific comments
  • Understand why double dispatch exists on the "Window unload flushes buffer" test
  • Support multiple calls to destroy.
  • Remove delay(1) from "Window unload flushes buffer", detect when unload handler is finished
  • Remove delay(1) from "FileHandler: Critical logs trigger immediate flush", detect when unload handler is finished
  • Either un-export LogQueue or move it to std/async/???.ts
  • Write more tests

Initial Idea:

  • Replacing BufWriterSync with Buffer from std/streams/buffer.ts
  • Using the magic line this._buf.readable.pipeTo(this._file.writable); to pipe the buffer to the file once full.
  • Change as little as possible

I'm not sure how much time I can dedicate to this over the next few weeks so if anyone wants to pick this up, you are welcome to.

Copy link
Contributor Author

@syhol syhol left a comment

Choose a reason for hiding this comment

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

Test are now passing.

Big changes since the last review are:

  • The log string is now encoded into a Uint8Array much earlier in the process, this means we only need to do it once rather than 2 or 3 times.
  • LogQueue is a new class which essentially converts synchronous calls to .enqueue into an AsyncIterable. We use this to make sure:
    1. All logs have been written when calling flush
    2. We only process one log line at a time making sure we avoid race conditions with either flushToBuffer or rotateLogFiles. I'm not 100% sure there would be race conditions now, but it could be easy to introduce in the future.
    3. See here for further justification
  • Flush is now async, that means destroy is now async, this has a knock-on effect on a few things, especially tests.
  • We now re-create the Buffer (not the ArrayBuffer just the wrapper) each time we flush to file. I'm not sure if it's intended behaviour but if we don't recreate the Buffer it fails to write any data after the first flush.

@iuioiua would you mind taking a look at the changes? I've added some questions and TODOs in comments in the code.

I know LogQueue might seem a tad over-engineered, but it feels like a really nice abstraction and could be used for other things (see the comment about moving it). If the logic was implemented inline in the FileHandler class it would just add complexity to an already complex class.

I'm open to critical feedback, and looking forward to progressing this PR. I'll try to spend a few more hours on it over the holidays.

@syhol
Copy link
Contributor Author

syhol commented Dec 24, 2023

Due to the fact that the unload lifecycle event handler does not await promises, using the async streams API is very risky as the last flush on unload would not complete.

As an alternative I've created this new PR #4021 that keeps to synchronous functions. It removes all std/io and Deno.Writer references and uses a plain ArrayBuffer and Uint8Array to buffer logs. Also it's a very small diff compared to this PR, only +22 −13.

@iuioiua
Copy link
Contributor

iuioiua commented Dec 28, 2023

Closing in favour of #4021.

@iuioiua iuioiua closed this Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants