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.

Sign up for GitHub

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

Jump to bottom

Add the TWebFile::Close(Option_t *option) method #13793

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Add the TWebFile::Close(Option_t *option) method #13793

merged 1 commit into from
Oct 17, 2023

Conversation

bellenot
Copy link
Member

@bellenot bellenot commented Oct 3, 2023

No description provided.

@bellenot
Add the TWebFile::Close(Option_t *option) method
e9ed3a7 
This is needed to properly close and delete the socket when closing the file in `TROOT::CloseFiles()`, preventing a potential issue when trying to close the socket afterwards
@bellenot bellenot self-assigned this Oct 3, 2023
@bellenot bellenot requested a review from gganis as a code owner October 3, 2023 14:15
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2023-10-03T16:10:31.733Z] ./etc\dictpch\allHeaders.h:1872:10: fatal error: 'ROOT/RCluster.hxx' file not found

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@bellenot
Copy link
Member Author

bellenot commented Oct 4, 2023

@phsft-bot build just on windows10/default

@phsft-bot
Copy link
Collaborator

Starting build on windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on windows10/default.
See console output.

@bellenot
Copy link
Member Author

bellenot commented Oct 4, 2023

@phsft-bot build just on windows10/default

@phsft-bot
Copy link
Collaborator

Starting build on windows10/default
How to customize builds

pcanal
pcanal reviewed Oct 4, 2023
View reviewed changes
net/net/src/TWebFile.cxx
Comment on lines +414 to +415
if (fSocket)
delete fSocket;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (fSocket)
delete fSocket;
delete fSocket;

pcanal
pcanal reviewed Oct 4, 2023
View reviewed changes
net/net/src/TWebFile.cxx
fSocket = nullptr;
if (fFullCache) {
free(fFullCache);
fFullCache = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fFullCache = 0;
fFullCache = nullptr;

@bellenot
Copy link
Member Author

bellenot commented Oct 4, 2023 •
edited
Loading

@pcanal thanks for the review. Since it is a backport, should I also change the master and then backport those changes?

@pcanal
Copy link
Member

pcanal commented Oct 16, 2023

Since it is a backport, should I also change the master and then backport those changes?

Yes, the change technically should be in the master. However they are NFC, so not urgent.

@bellenot
Copy link
Member Author

Since it is a backport, should I also change the master and then backport those changes?

Yes, the change technically should be in the master. However they are NFC, so not urgent.

So I can merge this PR and apply your required changes later on in master and backport them to 6-28-00-patches, right?

@pcanal
Copy link
Member

pcanal commented Oct 16, 2023

Yes.

pcanal
pcanal approved these changes Oct 16, 2023
View reviewed changes
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM. Further NFC can be done in a later PR.

@bellenot bellenot merged commit ebb1810 into root-project:v6-28-00-patches Oct 17, 2023
1 of 2 checks passed
@bellenot bellenot deleted the backport branch October 17, 2023 07:01
@hahnjo hahnjo mentioned this pull request Oct 18, 2023
Merged
@smuzaffar
Copy link
Contributor

@bellenot , @Axel-Naumann , we are trying to test latest ROOT 6.28 changes and our CI tests are failing with Too many open files and system logs shows [a].

Note that 96ca920 worked fine for CMSSW, it is just the last two commits on 6-28 patches branch which are causing this error. Could this PR be the reason for these failures?

[a]

Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded
Oct 19 15:23:09 cmsbuild120.cern.ch cvmfs2[6803]: (unpacked.cern.ch) open file descriptor limit exceeded

@pcanal
Copy link
Member

pcanal commented Oct 19, 2023

Not sure why yet, but indeed very likely related to the addition of TWebFile::Close.

@pcanal
Copy link
Member

pcanal commented Oct 19, 2023

The patch is semantically wrong (the new code should have been after the TFile::Close but I also can see why it would lead to the error ... and I can not reproduce it.

@bellenot
Copy link
Member Author

Sorry about that. I don't see how this change can lead to the errors you see, but anyway, how can I reproduce the error?

@smuzaffar
Copy link
Contributor

I am also not sure how TWebFile can cause this. I have tried to run tests multiple times 9 on different build nodes) and every time some random unit tests fail with same error (Too many open files). I am now running the tests without this change to see if they still fail or not

@bellenot
Copy link
Member Author

OK, thanks

@smuzaffar
Copy link
Contributor

smuzaffar commented Oct 23, 2023 •
edited
Loading

So looks like issue is not with this change. All ROOT versions 6.28 and above (i.e 6.28, 6.30 and master) open a lot of files (37K vs 4K) as compare to root 6.26. See cms-sw/cmssw#43077 issue.

@bellenot
Copy link
Member Author

OK, thanks @smuzaffar

All ROOT versions 6.28

You mean even with previous version of 6.28 (e.g. 6.28.06)?

@smuzaffar
Copy link
Contributor

yes , root 6.28 patches branch even before this change. I do not know when this behavior started (I am testing it in previous builds of root 628 releases to see if I can identify it)

@bellenot
Copy link
Member Author

OK, thanks @smuzaffar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcanal pcanal pcanal approved these changes

@vepadulano vepadulano Awaiting requested review from vepadulano

@gganis gganis Awaiting requested review from gganis gganis is a code owner

Assignees

@bellenot bellenot

Labels
pr:backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bellenot @phsft-bot @pcanal @smuzaffar