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

Reimplement PipedInput/OutputStream #2383

Merged
merged 20 commits into from
May 5, 2021
Merged

Reimplement PipedInput/OutputStream #2383

merged 20 commits into from
May 5, 2021

Conversation

vasilmkd
Copy link
Member

@vasilmkd vasilmkd commented May 3, 2021

Resolves #2037. I included the original reproduction as a test.

This was not pleasant to write.

@vasilmkd
Copy link
Member Author

vasilmkd commented May 3, 2021

The failing tests are already addressed in #2373.

*
* @param capacity the capacity of the allocated circular buffer
*/
private[io] final class InputOutputBuffer(private[this] val capacity: Int) { self =>
Copy link
Contributor

Choose a reason for hiding this comment

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

One general question: given that this entire class only depends on Java libraries, could it be submitted for the Scala standard library? Sys or process.

private[this] var closed: Boolean = false

private[this] val readerPermit: Semaphore = new Semaphore(1)
private[this] val writerPermit: Semaphore = new Semaphore(1)
Copy link
Member

@mpilquist mpilquist May 4, 2021

Choose a reason for hiding this comment

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

Could you add a summary of how these permits are used? Are their values ever > 1?

Copy link
Member Author

@vasilmkd vasilmkd May 4, 2021

Choose a reason for hiding this comment

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

I believe they can be > 1 at some point, they are not completely foolproof, they are just here to prevent threads from looping endlessly when there is nothing to be read from the buffer. This is fine, because there is one more monitor, the self.synchronized block, which actually guards the critical section. Does this make any sense?

Copy link
Member

Choose a reason for hiding this comment

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

Okay cool. My only concern is that some strange access pattern could result in writerPermit getting very large, which then causes lots of looping when waiting to write.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's not rush this, I will reiterate on the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented a custom synchronizer that allows at most 1 permit at any time.

Copy link
Member

Choose a reason for hiding this comment

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

Looks great!

@vasilmkd vasilmkd marked this pull request as draft May 4, 2021 14:08
@vasilmkd vasilmkd marked this pull request as ready for review May 5, 2021 11:11
@vasilmkd
Copy link
Member Author

vasilmkd commented May 5, 2021

Wow the AbstractQueuedSynchronizer API is something else...

@mpilquist mpilquist merged commit f882642 into typelevel:main May 5, 2021
@vasilmkd vasilmkd deleted the iobuffer branch May 5, 2021 12:19
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.

readOutputStream binds the OutputStream to the writing thread's lifetime
3 participants