You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2023/01/17 15:12:37 UTC
[GitHub] [thrift] cfriedt opened a new pull request, #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
cfriedt opened a new pull request, #2745:
URL: https://github.com/apache/thrift/pull/2745
<!-- Explain the changes in the pull request below: -->
Close a file descriptor leak in TServerSocket::close(). This was discovered in the final stages of porting Thrift to the Zephyr RTOS, which has a finite-sized file descriptor table.
Details at the URL below.
https://issues.apache.org/jira/browse/THRIFT-5677
cc: @Jens-G
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cfriedt commented on pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
Posted by GitBox <gi...@apache.org>.
cfriedt commented on PR #2745:
URL: https://github.com/apache/thrift/pull/2745#issuecomment-1385689183
> @cfriedt , I guess this is beyond your current scope, but for the sake of clean code, would it not be better to bind the `std::shared_ptr` to a custom destructor that calls `::THRIFT_CLOSESOCKET` on the socket upon destruction?
Maybe - that sounds a bit more complicated.
> And then again, why is the operating system not doing that in the first place?
I suspect that is exactly the issue - devs might expect the OS to close dangling FDs for them. However, neither Linux nor macOS do this either (not before the process ends). I have not checked what the status is for Windows.
> Please let me know if I'm completely on the wrong track here...
I would guess yea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cfriedt commented on pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
Posted by GitBox <gi...@apache.org>.
cfriedt commented on PR #2745:
URL: https://github.com/apache/thrift/pull/2745#issuecomment-1385690865
At first glance, it seems like the build failures are unrelated, but I could take a deeper dive if I'm mistaken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cfriedt commented on pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
Posted by GitBox <gi...@apache.org>.
cfriedt commented on PR #2745:
URL: https://github.com/apache/thrift/pull/2745#issuecomment-1396431623
@emmenlau - interesting, it seems that there is already a custom deleter, so perhaps it's the C++ runtime that is the problem.
```
pChildInterruptSockReader_
= std::shared_ptr<THRIFT_SOCKET>(new THRIFT_SOCKET(sv[0]), destroyer_of_fine_sockets);
```
I think I may close this PR, as it seems that could be the problem. Sorry for the trouble.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] emmenlau commented on pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
Posted by GitBox <gi...@apache.org>.
emmenlau commented on PR #2745:
URL: https://github.com/apache/thrift/pull/2745#issuecomment-1385668209
@cfriedt , I guess this is beyond your current scope, but for the sake of clean code, would it not be better to bind the `std::shared_ptr` to a custom destructor that calls `::THRIFT_CLOSESOCKET` on the socket upon destruction? And then again, why is the operating system not doing that in the first place?
Please let me know if I'm completely on the wrong track here...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] cfriedt closed pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
Posted by GitBox <gi...@apache.org>.
cfriedt closed pull request #2745: lib: cpp: transport: file descriptor leak in TServerSocket::close()
URL: https://github.com/apache/thrift/pull/2745
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org