search

pact-foundation / pact-plugins

🏰 Architecture to support Plugins 🔌 with Pact 🔗
MIT License
19 stars 11 forks source link

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

Closed YOU54F closed 1 month ago

YOU54F commented 3 months ago

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

The root issue cause of the issue https://github.com/tokio-rs/tokio/issues/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)

rholshausen commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

https://github.com/GuillaumeGomez/sysinfo/pull/1341

YOU54F commented 2 months ago

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 https://github.com/pact-foundation/pact-plugins/pull/69

YOU54F commented 2 months ago

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