Not thread safe file manipulation results in build failures of multi-module projects

[nikita2206]

Maven Version

3.9.8

Version

0.0.8

Sentry SDK Version

7.14.0

Steps to Reproduce

1. Have a multi-module Maven project
2. Run mvn install -T1C (or higher concurrency)

This breaks non-deterministically

Expected Result

Sources are uploaded to Sentry

Actual Result

Getting an error

/bin/sh: 1: /tmp/.sentry-cli10465974399318709029.exe: Text file busy

As far as I understand it is coming from this line

sentry-maven-plugin/src/main/java/io/sentry/cli/SentryCliRunner.java

Line 75 in daa4fd9

escape(getCliPath(mavenProject, sentryCliExecutablePath))

Which is preceded by

sentry-maven-plugin/src/main/java/io/sentry/SentryCliProvider.java

Line 112 in daa4fd9

final @NotNull File tempFile = File.createTempFile(".sentry-cli", ".exe");

After reviewing how thread safe the implementation is, I couldn't pinpoint where the issue is.
Not having other ideas, it seems like the root cause may lie in the JVM bug: https://bugs.openjdk.org/browse/JDK-8068370

It is suggested that a retry could help with that issue. Would be happy to accept a PR that implements a single-attempt retry logic for this bit of code?

[adinauer]

Thanks for opening this issue. We haven't optimized the sentry-maven-plugin for parallel execution (yet).

We could ensure only a single SentryCliRunner is being used. I'm not certain this is possible as I haven't read up on how parallelism works in Maven. If everything is running inside a single process where the classloader is shared, it shouldn't be a problem to do so.

We could then synchronize runSentryCli to only have one invocation of sentry-cli running at any time.

I'm afraid merely retrying would only fix the problem in a few cases but not all of them and it'd just take longer to fail - not sure that's much better than the current behaviour.

Or do I misunderstand the issue and it's only a problem right after sentry-cli is copied to a temporary location and unrelated to multiple invocations of it?

We're gonna take a look at this in more detail but I can't give an ETA at this point, as we're currently working on v8 of the Java SDK.

[nikita2206]

Thanks for your reply @adinauer

This seems to be happening right after the file is copied, this happens only sometimes. Seems like the exact way to reproduce this is to create a file, mark it executable and then attempt to run it as a subprocess. Given this, it seemed like there's just no way around this bug given the File System features that JVM exposes/doesn't expose.

One other option I'm considering is ignoring the failure in Maven, because being unable to upload sources isn't really a show stopper but only an inconvenience.

[nikita2206]

With the above in mind @adinauer , what do you think about adding the following parameter, which is also part of a few other Maven plugins:

<configuration>
  <failOnError>false</failOnError>
  ...

If you think it's acceptable for you guys, I can quickly open a PR.

Otherwise, we are quite happy with how errors looks now in Sentry 🙌

[adinauer]

That sounds good, only downside is probably that on a fail you have to upload the sources somehow. I'm not quite sure how that'd play out exactly. On a later upload you just need to make sure you use the same ID that's been packaged with the JAR as io.sentry.bundle-ids in sentry-debug-meta.properties.

We can tackle that problem separately as I'm not sure how exactly we wanna solve it.

A PR would be very welcome! Thanks for the feedback!

[adinauer]

Regarding the original problem here, maybe it's sufficient to just sleep for a bit and only then use the freshly copied file.

[nikita2206]

I've realized something basic that I oversaw initially, the reason this was happening is that the plugin tries to source the binary/script from its own JAR, and we can tell it to not do that and provide it with our own sentry-cli. That was my workaround, so I'm pre-downloading the sentry-cli (currently on a separate build step, but soon will bake it as part of the build image), and configuring the plugin with <sentryCliExecutablePath>${env.SENTRY_CLI_PATH}</sentryCliExecutablePath> to tell it to use that binary.

As for the root cause fix, I think you're right and a short sleep might actually help, I'd try that.