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

upstream: remove dependency on cfriedt/thrift and depend on apache/thrift directly #158

Merged
merged 20 commits into from
Jan 23, 2023

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Nov 20, 2022

This changeset actually has a fairly big impact - it removes the dependency on cfriedt/thrift (a fork) and enables the module to use apache/thrift directly 😀

With that, we will also be able to pin to a specific release of apache/thrift, likely the upcoming v0.18.0 release.

The only remaining issue now is to fix std::thread and std::mutex in Zephyr. As a temporary workaround, we've copied in the affected apache/thrift sources and headers and made some slight modifications. Eventually, when Zephyr supports std::thread and std::mutex, we can remove those workarounds.

See individual commit messages for details.

@cfriedt cfriedt requested a review from stephanosio November 20, 2022 14:44
@cfriedt cfriedt self-assigned this Nov 20, 2022
@cfriedt
Copy link
Member Author

cfriedt commented Nov 20, 2022

@cfriedt cfriedt changed the title Switch to apache thrift master switch to apache thrift master Nov 20, 2022
@cfriedt cfriedt mentioned this pull request Nov 20, 2022
25 tasks
@cfriedt cfriedt changed the title switch to apache thrift master upstream: remove dependency on cfriedt/thrift and depend on apache/thrift directly Nov 20, 2022
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch 5 times, most recently from 490103b to 7884727 Compare January 15, 2023 10:06
@cfriedt
Copy link
Member Author

cfriedt commented Jan 15, 2023

@cfriedt
Copy link
Member Author

cfriedt commented Jan 17, 2023

Need to update git submodule commit after apache/thrift#2745 is merged, as it appears that TSocketServer.cpp (upstream) leaks file descriptors.

For more details, see
https://issues.apache.org/jira/browse/THRIFT-5677

cc @stephanosio @SdtElectronics

Young, you may have encountered this in your SSL work.

@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch from 4d7f766 to 75c32d9 Compare January 19, 2023 05:00
@cfriedt
Copy link
Member Author

cfriedt commented Jan 19, 2023

Need to update git submodule commit after apache/thrift#2745 is merged, as it appears that TSocketServer.cpp (upstream) leaks file descriptors.

I was wrong - it seems that custom deleters are not working in Zephyr's C++ runtime. If that were the case, then destroyer_of_fine_sockets() would be called.

https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TServerSocket.cpp#L91
https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TServerSocket.cpp#L407

Kind of an interesting C++ quirk @stephanosio. Might be good to add a test for that. I'm sure it's a lot more involved under the hood, of course ;-)

@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch 3 times, most recently from 5e6f3a5 to e8666fd Compare January 20, 2023 16:00
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch from e8666fd to ae52a98 Compare January 20, 2023 16:02
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch 2 times, most recently from 9435a56 to 184bcb3 Compare January 20, 2023 19:47
The Thrift sources put includes directly in the source directory.

We'll need to override some upstream headers to workaround
Zephyr's lack of support for `std::thread` and `std::mutex`, so
our local include paths should come first.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
Zephyr does not yet support `std::thread` or `std::mutex`.

Fixes #28

Signed-off-by: Chris Friedt <cfriedt@meta.com>
Upstream does not have a separate include directory.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch from acc6e57 to 81a053e Compare January 21, 2023 17:21
This file is not required.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Just to keep things slightly separate.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
With this, along with upstream Zephyr POSIX fixes, we can run
perfectly against upstream Thrift.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
This is no longer needed after upstream Zephyr POSIX
fixes.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Previously, there was an issue in the Zephyr SDK that prevented
some targets from building properly. However, we do want to
ensure that we have coverage for at least arm, aarch64, riscv,
riscv64, x86, and x86_64, so re-enable those targets here.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
The system workqueue thread had experienced a stack overflow for
BOARD=`qemu_riscv64_smp`. Bump it for now for all platforms.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
This was fixed with release 0.15.2 of the SDK but somehow
`qemu_x86` was not included. Once a newer version of the SDK
is released with a fix, we can re-enable `qemu_x86` as an
integration platform in Thrift.

The exact details are highlighted in Zephyr SDK Issue 603.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
Since the file descriptor leak was plugged in the previous
commit, we can reduce memory requirements for this test.

Also, we can keep the stack sentinal as default.

Lastly, use `qemu_riscv64` instead of `qemu_riscv64_smp`
for now, as the latter seems to be experiencing weird issues.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
Upstream has an issue in that `TConnectedClient` has a non-
virtual destructor. This results in a warning from GCC
when compiling `TServerFramework.cpp`. Of course, in CI,
all of Zephyr's warnings are promoted to errors, which
will break the build.

Tracking the issue upstream at the link below.

https://issues.apache.org/jira/browse/THRIFT-5678

We'll keep this workaround in tree until the fix is
merged upstream.

Fixes #160

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
It would seem that Zephyr's C++ runtime was not properly
executing custom deleters for `std::shared_ptr`. Originally,
it seemed as though `TServerSocket.cpp` was leaking file
descriptors, but there is a custom deleter used that is
never called. Details at the URL below.

https://issues.apache.org/jira/browse/THRIFT-5677

Once this is fixed in Zephyr's C++ runtime, this workaround
should be reverted.

Fixes #161

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Tested all configurations against programes compiled for the host
which are built with `.../hello_client/Makefile`.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
Tested all configurations against programes compiled for the host
which are built with `.../hello_server/Makefile`.

Signed-off-by: Christopher Friedt <cfriedt@meta.com>
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch from 81a053e to 4421837 Compare January 21, 2023 17:22
Like Zephyr's CI, there are occasionally statistical outliers,
race conditions, random timing changes, particularly on SMP
platforms that can make a test fail.

Retry a test failure up to 3 times before failing in CI.

Signed-off-by: Chris Friedt <cfriedt@meta.com>
@cfriedt cfriedt force-pushed the switch-to-apache-thrift-master branch 2 times, most recently from 96f692f to 9ce1006 Compare January 23, 2023 18:16
@cfriedt
Copy link
Member Author

cfriedt commented Jan 23, 2023

  • removed staging commit to build against Zephyr PR 51771 (so building against main again)

@cfriedt
Copy link
Member Author

cfriedt commented Jan 23, 2023

IIRC Stephanos is off for the next couple of days, so opening this up to other reviewers.

@cfriedt cfriedt merged commit e12e014 into main Jan 23, 2023
@cfriedt cfriedt deleted the switch-to-apache-thrift-master branch January 24, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants