Skip to content

TarReader should dispose underlying stream if leaveOpen is false - #79899

Merged
adamsitnik merged 1 commit into
dotnet:mainfrom
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged

TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom
jozkee:tarreader_leaveopen_false_should_dispose

Conversation

@jozkee

@jozkeejozkee commented Dec 22, 2022

Copy link
Copy Markdown
Member

Potentially fixes#77012.

@jozkeejozkee added this to the 8.0.0 milestone Dec 22, 2022
@jozkee
jozkee requested review from a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot mentioned this pull request Dec 22, 2022
Closed
adamsitnik
adamsitnik approved these changes Dec 22, 2022
View reviewed changes

@adamsitnikadamsitnik left a comment

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.

LGTM, thank you for a quick fix @jozkee !

@adamsitnik
adamsitnik merged commit 5090041 into dotnet:mainDec 22, 2022
@akoeplinger

akoeplinger commented Dec 22, 2022

Copy link
Copy Markdown
Member

should this be backported?

@akoeplingerakoeplinger mentioned this pull request Dec 22, 2022
Closed
@adamsitnikadamsitnik mentioned this pull request Dec 22, 2022
Closed
@jozkee
jozkee deleted the tarreader_leaveopen_false_should_dispose branch December 22, 2022 17:26
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger mentioned this pull request Dec 23, 2022
Merged
adamsitnik pushed a commit that referenced this pull request Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub reviewed Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
if (!_leaveOpen)
{
foreach (Stream s in _dataStreamsToDispose)
_archiveStream.Dispose();

@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.

@carlossanlopcarlossanlopJan 3, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.

Edit: I see my mistake, I misread the diff.

@jozkeejozkeeJan 11, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.

This is the opposite of what I did here. @carlossanlop shall we fix it?

@carlossanlopcarlossanlopJan 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes.

@jozkeejozkee mentioned this pull request Jan 12, 2023
Merged
jozkee added a commit to jozkee/runtime that referenced this pull request Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit to jozkee/runtime that referenced this pull request Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee mentioned this pull request Jan 13, 2023
Merged
carlossanlop pushed a commit that referenced this pull request Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free

5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
TarReader should dispose underlying stream if leaveOpen is false by jozkee · Pull Request #79899 · dotnet/runtime · GitHub
Skip to content
TarReader should dispose underlying stream if leaveOpen is false - #79899
Merged
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Dec 22, 2022
Merged
TarReader should dispose underlying stream if leaveOpen is false#79899
adamsitnik merged 1 commit into
dotnet:mainfrom 
jozkee:tarreader_leaveopen_false_should_dispose
Conversation
@jozkee
@jozkeejozkee
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
Potentially fixes#77012.
@jozkeejozkee
added this to the 8.0.0 milestone
Dec 22, 2022
@jozkee
jozkee
requested review from
a team, adamsitnik and carlossanlopDecember 22, 2022 03:56
@ghostghost
assigned jozkeeDec 22, 2022
@build-analysisbuild-analysisBot
mentioned this pull request
Dec 22, 2022

Closed

adamsitnik
adamsitnik
approved these changes
Dec 22, 2022
View reviewed changes
@adamsitnikadamsitnik
left a comment


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.

LGTM, thank you for a quick fix @jozkee !
@adamsitnik
adamsitnik
merged commit 5090041
into
dotnet:mainDec 22, 2022
@akoeplinger
akoeplinger
commented
Dec 22, 2022

Copy link


Copy Markdown

Member
should this be backported?
@akoeplingerakoeplinger
mentioned this pull request
Dec 22, 2022

Closed

@adamsitnikadamsitnik
mentioned this pull request
Dec 22, 2022
 Closed

@jozkee
jozkee
deleted the

tarreader_leaveopen_false_should_dispose

branch
December 22, 2022 17:26
akoeplinger added a commit
to akoeplinger/runtime
that referenced
this pull request
Dec 22, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well
732a5dc
Same as dotnet#79899
@akoeplingerakoeplinger
mentioned this pull request
Dec 23, 2022
 Merged

adamsitnik pushed a commit
that referenced
this pull request
Dec 23, 2022
@akoeplinger
Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
372adbf
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
stephentoub
stephentoub
reviewed
Jan 3, 2023
View reviewed changes
Comment threadsrc/libraries/System.Formats.Tar/src/System/Formats/Tar/TarReader.cs
 if (!_leaveOpen)
 {
 foreach (Stream s in _dataStreamsToDispose)
 _archiveStream.Dispose();
@stephentoubstephentoubJan 3, 2023

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.

What's the relationship between the streams in _dataStreamsToDispose and _archiveStream? Do the former streams wrap the latter? I'm wondering if the archive stream should actually be disposed of after rather than before the _dataStreamsToDispose.
@carlossanlopcarlossanlopJan 3, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The code change is correct incorrect: The archive stream represents the whole tar archive, while the data streams represent sub-sections of the archive, each wrapping only the data section of an individual entry.
This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
Edit: I see my mistake, I misread the diff.
@jozkeejozkeeJan 11, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This means that all the individual data streams need to get disposed first, and the archive stream needs to be disposed at the end.
This is the opposite of what I did here. @carlossanlop shall we fix it?
@carlossanlopcarlossanlopJan 11, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Yes.
@jozkeejozkee
mentioned this pull request
Jan 12, 2023
 Merged

jozkee added a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@jozkee
TarReader should dispose underlying stream if leaveOpen is false (dot…
08e8e19
…net#79899)
jozkee pushed a commit
to jozkee/runtime
that referenced
this pull request
Jan 13, 2023
@akoeplinger@jozkee
Dispose underlying stream in TarReader.DisposeAsync() as well (dotnet…
1fbde51
…#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as dotnet#79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
@jozkeejozkee
mentioned this pull request
Jan 13, 2023
 Merged

carlossanlop pushed a commit
that referenced
this pull request
Jan 13, 2023
@jozkee@akoeplinger
[release/7.0] TarReader should dispose underlying stream if leaveOpen…
c8a73af
… is false (#80598)
* TarReader should dispose underlying stream if leaveOpen is false (#79899)
* Dispose underlying stream in TarReader.DisposeAsync() as well (#79920)
* Dispose underlying stream in TarReader.DisposeAsync() as well
Same as #79899
* Consolidate duplicated WrappedStream test helpers to Common sources
* Dispose stream passed to WrappedStream
* Dispose archive stream after the list of DataStreams (#80572)
* Dispose archive stream after the list of DataStreams
* Add tests for TarReader.DisposeAsync properly disposing underlying stream
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghostghost
locked as resolved and limited conversation to collaborators
Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@adamsitnikadamsitnikadamsitnik approved these changes
@carlossanlopcarlossanlopAwaiting requested review from carlossanlop

Assignees

@jozkeejozkee

Labels

area-System.Formats.Tar

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

TarWriter_WriteEntry_LongFile_Tests require 104GB free


5 participants

@jozkee@akoeplinger@carlossanlop@stephentoub@adamsitnik