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

plugin driver - switch to threads instead of tokio for loading plugins #68

Closed
YOU54F opened this issue Jul 14, 2024 · 5 comments · Fixed by #69
Closed

plugin driver - switch to threads instead of tokio for loading plugins #68

YOU54F opened this issue Jul 14, 2024 · 5 comments · Fixed by #69

Comments

@YOU54F
Copy link
Member

YOU54F commented Jul 14, 2024

There is an issue affecting Pact-Go users, whereby using plugins will cause sporadic segmentation faults.

The root issue cause of the issue tokio-rs/tokio#3520 - Signals do not add SA_ONSTACK which may cause linked Go applications to crash

We can switch to using std::process in rust, introduced in this commit for windows, which uses threads instead of tokio tasks

threaded version

tokio version

I've tested this out on my fork and it's resulting in stable ci runs for pact-go cross platform utilising the following pact plugins (avro, csv, protobuf, matt)

Note: Avro plugin has failed to load on windows since moving to threads in commit

Amending the following logback.xml from the installed pact-avro-plugin

From

https://github.com/austek/pact-avro-plugin/blob/main/modules/plugin/src/main/resources/logback.xml

To

<configuration>
</configuration>

will allow the plugin framework to pick up the plugins loaded message (serverKey and port).

So there may be a change required in the pact-avro-plugin to support.

This can easily be reproduced with on a windows machine by performing pact-plugin-cli install -y avro

It would be nice to implement the plugin timeout for loading - todo here by an env var possibly (so its usable by ffi & cli users easily)

YOU54F added a commit to YOU54F/pact-plugins that referenced this issue Jul 17, 2024
…l os

fixes tokio-rs/tokio#3520

Signals do not add SA_ONSTACK which may cause linked Go applications to crash.

fixes pact-foundation#68
fixes pact-foundation/pact-go#269

note: std::process unable to read pact-avro-plugin startup message see pact-foundation#68 for detail
YOU54F added a commit to YOU54F/pact-plugins that referenced this issue Jul 17, 2024
…l os

fixes tokio-rs/tokio#3520

Signals do not add SA_ONSTACK which may cause linked Go applications to crash.

fixes pact-foundation#68
fixes pact-foundation/pact-go#269

note: std::process unable to read pact-avro-plugin startup message see pact-foundation#68 for detail
@rholshausen
Copy link
Contributor

So where have we got with this? Do we still want to go ahead? Or should we wait until the startup issues are resolved?

@YOU54F
Copy link
Member Author

YOU54F commented Aug 21, 2024

still need to sort the plugin startup issue as that will propagate to macos/linux. the flush suggestion you mentioned, should hopefully resolve that. will check later today

then there is a sep issue about the plugin shutdown on windows for avro. but we aren’t any worse than today.

should we make it configurable so the user can choose to use tokio tasks or threads rather than just binning the tokio tasks off. i haven’t done any measurements to ascertain if either is slower

@YOU54F
Copy link
Member Author

YOU54F commented Aug 21, 2024

shutdown issue resolved with local testing, PR raised here, need to make a couple of changes

GuillaumeGomez/sysinfo#1341

YOU54F added a commit to YOU54F/pact-plugins that referenced this issue Aug 21, 2024
…l os

fixes tokio-rs/tokio#3520

Signals do not add SA_ONSTACK which may cause linked Go applications to crash.

fixes pact-foundation#68
fixes pact-foundation/pact-go#269

note: std::process unable to read pact-avro-plugin startup message see pact-foundation#68 for detail
@YOU54F
Copy link
Member Author

YOU54F commented Aug 21, 2024

fixed the startup issues now, woohoo!

https://github.com/austek/pact-avro-plugin/pull/45/files

we lose the rolling file appender from the plugin for reasons unknown.

pact plugin PR for this issue is updated here #69

@YOU54F
Copy link
Member Author

YOU54F commented Aug 30, 2024

pact-avro-plugin pr merged and released as 0.0.6 which gets us starting up

https://github.com/pact-foundation/pact-go/actions/runs/10637661001/job/29491970199

need #69 merging which will fix #73

YOU54F added a commit that referenced this issue Aug 30, 2024
relates to tokio-rs/tokio#3520

Signals do not add SA_ONSTACK which may cause linked Go applications to crash.

fixes #68
fixes pact-foundation/pact-go#269

windows plugin shutdowns, terminate child processes
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 a pull request may close this issue.

2 participants