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

Fix #2484 - clear m_fill on flush exception #2485

Merged
merged 1 commit into from
Oct 25, 2020

Conversation

tchaloupka
Copy link
Contributor

Well it's a workaround for this particular issue.

Problem is that in StreamOutputRange destructor flush() is called to make sure that data has been written.
But when exception is thrown in it, destructor won't cleanup what'll normally be cleaned up and so the leak.

I've fixed it by resetting m_fill in a flush itself but it works only when written data are bigger than the internal buffer so flush is called and fails before the destructor.
For this case to work I've added manual flush() to doWriteJsonBody() so it's called before the destructor.

But there are much more places this can cause random problems and I'm not sure what would be the best approach for this. But throwing exceptions in destructors or allocating in them can easilly lead to InvalidMemoryOperation when called from GC (which is not this case).

@tchaloupka
Copy link
Contributor Author

One way we can workaround this for all StreamOutputRange usages is with something like this:

~this()
{
    static if (hasElaborateDestructor!OutputStream) scope(failure) destroy(m_stream);
    flush();
}

@tchaloupka
Copy link
Contributor Author

I've changed the patch of StreamOutputRange cleanup to work in all cases.
For reference I've also created https://issues.dlang.org/show_bug.cgi?id=21322

@s-ludwig
Copy link
Member

The workaround sounds good for now, I don't really see what else could be done here, apart from just asserting the the range has been flushed. But this range was meant to be supported by RAII, so that would be quite unfortunate.

We can drop the scope (exit) m_fill = 0; with the scope (failure) workaround, though, right? Semantically it seems nicer in the sense that it could allow to retry the transfer (although whether the outcome is well defined, obviously depends on the underlying stream implementation).

@tchaloupka
Copy link
Contributor Author

Ok, I've reverted the m_fill reset in flush(). Only problem with that is that when exception is thrown in flush() then it would be thrown again in the destructor itself in this case.

@s-ludwig
Copy link
Member

True, that may be unexpected. A flag that skips the destructor flush whenever the last operation was not successful looks like it would result in the expected behavior. But I'd open a quick separate PR for that, to first fix the important part here.

@s-ludwig
Copy link
Member

I've opened the follow-up PR: #2489

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.

2 participants