Skip to content

Restore thread interrupt status in InterruptedException catch blocks - #40275

Open
TimurRakhmatullin86 wants to merge 1 commit into
apache:masterfrom
TimurRakhmatullin86:fix/restore-interrupt-status
Open

TimurRakhmatullin86 wants to merge 1 commit into
apache:masterfrom
TimurRakhmatullin86:fix/restore-interrupt-status

Conversation

@TimurRakhmatullin86

Copy link
Copy Markdown

Summary

When a thread catches InterruptedException, the JVM clears the thread's interrupt flag. Per Java concurrency best practices (Java Concurrency in Practice, Ch. 7), the catch block must either re-throw the InterruptedException or restore the flag via Thread.currentThread().interrupt(). Failing to do so silently swallows the interrupt signal, which can cause the thread (and its callers) to miss shutdown or cancellation requests — a particularly dangerous bug in a distributed data processing framework like Beam where pipeline cancellation relies on interrupt propagation.

This PR fixes 33 occurrences across 27 files where InterruptedException was caught without restoring the interrupt status.

Affected modules

Area Files
Core SDK TransformUpgrader, BeamFnDataGrpcMultiplexer
I/O Connectors Kafka (KafkaUnboundedReader), Kinesis (ShardReadersPool), BigQuery (BigQueryStorageStreamSource, DynamicDestinationsHelpers), Spanner (SpannerIO), FHIR (FhirIO), Hadoop (HadoopFormatIO), Spark Receiver (ReadFromSparkReceiverWithOffsetDoFn), Request-Response (Call, Repeater)
Runners Flink (SplittableDoFnOperator, StreamingImpulseSource x2), Jet (JetPipelineResult), Prism (PrismExecutor), fn-execution (GrpcDataService, DockerCommand, BeamWorkerStatusGrpcService)
Extensions Python (PythonService, PythonExternalTransform), GCP (RetryHttpRequestInitializer)
Harness FinalizeBundleHandler
Transform Service ExpansionService, TransformServiceLauncher
Testing Nexmark (NexmarkLauncher — used Thread.interrupted() which clears the flag instead of Thread.currentThread().interrupt() which sets it)

Categories of fixes

  1. Silent swallow — catch blocks with // ignore comment that discarded the interrupt entirely
  2. Multi-catch conflation — catch (IOException | InterruptedException e) blocks that handled InterruptedException identically to IOException, losing the interrupt semantics. Fixed by adding if (e instanceof InterruptedException) Thread.currentThread().interrupt();
  3. Wrap-and-throw without restore — catch blocks that wrapped in RuntimeException/IllegalStateException and threw, but didn't restore the interrupt flag first
  4. Wrong API — NexmarkLauncher called Thread.interrupted() (a static method that reads and clears the flag) instead of Thread.currentThread().interrupt() (which sets the flag)

Test plan

  • Existing unit tests continue to pass (these changes only add Thread.currentThread().interrupt() calls, which is a no-op if the thread is not being interrupted)
  • No behavioral change for non-interrupted threads
  • Interrupted threads now correctly propagate their interrupt status to callers

🤖 Generated with Claude Code

When a thread catches InterruptedException, the interrupt flag is
cleared by the JVM. Per Java concurrency best practices, the catch
block must either re-throw the InterruptedException or restore the
flag via Thread.currentThread().interrupt(). Failing to do so silently
swallows the interrupt signal, which can cause the thread (and its
callers) to miss a shutdown or cancellation request.

This change fixes 33 occurrences across 27 files in the Beam codebase
where InterruptedException was caught without restoring the interrupt
status. The affected modules span core SDK, I/O connectors (Kafka,
Kinesis, BigQuery, Spanner, FHIR, Hadoop, Spark Receiver, Solr), runner
integrations (Flink, Jet, Prism, fn-execution), extensions (Python,
GCP), harness, and the transform service.

Fixes fall into several categories:
- Catch blocks that silently swallowed the interrupt ("// ignore")
- Multi-catch blocks (e.g. IOException | InterruptedException) that
  handled InterruptedException the same as IOException
- Catch blocks that wrapped and re-threw as RuntimeException without
  first restoring the interrupt flag
- One case (NexmarkLauncher) that called Thread.interrupted() (which
  clears the flag) instead of Thread.currentThread().interrupt()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Comment on lines 611 to 614
} catch (InterruptedException e) {
LOG.warn("{}: consumer thread is interrupted", this, e); // not expected
Thread.currentThread().interrupt();
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This block only catches interrupts that did not originate from a Kafka client (preexisting bug).
Kafka clients do not throw InterruptedException, but they may throw their own wrapper exception InterruptException instead.

No action required on this PR.
Just making sure I won't forget to address it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants