#903 jdp-2026-01: Additional locking for sending in wire protocol

Author: mrotteveel

devdoc/jdp/jdp-2026-01-additional-locking-for-sending-in-wire-protocol.adoc

@@ -0,0 +1,111 @@
+= jdp-2026-01: Additional locking for sending in wire protocol
+
+// SPDX-FileCopyrightText: Copyright 2026 Mark Rotteveel
+// SPDX-License-Identifier: LicenseRef-PDL-1.0
+
+== Status
+
+* Draft
+* Proposed for: Jaybird 7
+
+== Type
+
+* Feature-Specification
+
+== Context
+
+With Jaybird 3, we implemented support for protocol 12 and cancellation.
+We wanted to avoid an overly complex solution with overlapping locks to perform separate locking for sending and receiving, or switching to NIO.
+
+We decided to take the chance and implement cancellation in a way that is not thread-safe, by sending the cancellation message without taking out the connection lock.
+(The connection lock covers sending and receiving, making cancellation only occur after the operation if you try it under that lock.)
+This can lead to the cancellation message being sent in the middle of a normal message.
+This then results in the server receiving a corrupt message, with undefined results (e.g. errors, the server doing the wrong thing, or the connection being terminated).
+We tried to reduce this risk by first constructing the message on a separate `XdrOutputStream`, and then sending the resulting byte array at once.
+
+We've now received reports that indicate this can occur when performing rapid cancellation, where the cancellation might overlap with the statement execute (or another action).
+We've been able to produce a broken connection with rapid cancellation;
+this was a different issue than reported, though.
+
+The native implementation is not affected (and if it were, it is probably not an issue we can address within Jaybird).
+
+== Decision
+
+As we now have a clear case that our not thread-safe solution can actually fail in production, we will need to address the thread-safety of cancellation.
+
+For the wire protocol, we will add a "`transmit`"-lock which should _only_ cover transmitting (sending) messages to the server.
+This lock should _not_ cover receiving messages, and preferably not cover other actions unrelated to sending messages to the server.
+The "`transmit`"-lock should be held for the sending of an entire message.
+When multiple messages are batched without an interleaved receive of responses, it should be considered carefully if the lock should cover individual messages or the entire batch.
+
+For normal operations, the "`transmit`"-lock should be taken out while the normal connection lock is held (i.e. within a try-with-resources calling `withLock()`).
+For the cancellation operation, and possibly other "`out-of-band`" transmissions, only the "`transmit`"-lock should be taken out;
+this can probably be done within implementations of `FbWireOperations.writeDirect(byte[])`.
+
+The lock will use a `java.util.concurrent.locks.ReentrantLock` (see also <<rejected>>).
+
+Exact details of use will be fleshed out during implementation, but `XdrStreamAccess` is probably the right place to provide access to this lock.
+We will provide two methods of taking out the lock:
+
+. `LockCloseable withTransmitLock()` -- for use with try-with-resources in the same fashion as the `withLock()` method(s) for the connection lock
+. `void withTransmitLock(TransmitAction transmitAction) throws SQLException, IOException` where `TransmitAction` is a functional interface with a method `void transmit(XdrOutputStream xdrOut) throws SQLException, IOException`
++
+If possible, the `IOException` should be left out of the throws lists of `withTransmitLock(TransmitAction)` and maybe `transmit(XdrOutputStream)` by using `FbExcepionBuilder.ioWriteError(IOException)`, but right now we can't oversee if that is always possible.
+On the other hand, we could always fall back on the first option if there are cases where an `IOException` must be thrown.
+
+If needed, implementation classes (but not the `FbWireXXX` interfaces) may provide the same methods (e.g. as `protected`) for purposes of code simplication, as long as they ultimately call the actual locking methods.
+
+The latter should also be viewed as an experiment if in the future we can use a similar alternative for `withLock()`.
+If during implementation this turns out to be too cumbersome, this may need to be revisited, and maybe we'll only implement the first option.
+
+During implementation, this will be done first in the `FbWireStatement` implementations and the `FbWireOperations.writeDirect(byte[])` method as a proof of concept.
+If this proof of concept fails to resolve the broken connection issue we can reproduce, we will need to rethink this approach.
+
+Given the invasiveness of this change, it will not be backported to Jaybird 5 and 6.
+We will consider a reduced solution, only covering `FbWireStatement` implementations, to address the immediate thread-safety problem of statement cancellation.
+Such a reduced solution could still cause problems if statement cancellation is done while the connection is performing a non-statement operation.
+If we decide to go for such a solution for Jaybird 5 and/or 6, it will be covered by a separate JDP.
+
+[#rejected]
+=== Rejected options
+
+As the lock is generally uncontested, we considered using a simple spinlock on an `AtomicBoolean` or a volatile `int` field (with help of `AtomicIntegerFieldUpdater`) instead of a full-blown lock.
+Unfortunately, there may be cases where we need reentrancy of the lock due to implementation of protocol versioning (e.g. an overloaded method calling its parent and then doing something that should be done within the "`transmit`"-lock).
+
+If during implementation it turns out that this is not a problem (e.g. because the lock can be taken out by the callers of such overloaded methods), we can revisit this decision.
+
+== Consequences
+
+The entire wire protocol implementation is revised to take out the "`transmit`"-lock during sending.
+
+The javadoc of `FbWireOperations.writeDirect(byte[])` is revised to remove mention of race conditions.
+The javadoc of other methods, etc., are revised as needed.
+
+"`Normal`" users of the JDBC driver and the internal GDS-ng API, are not affected -- there is no change in the methods they call, or the operation or behaviour of the driver (except cancellation is now thread-safe).
+
+Implementers of Jaybird forks or alternative wire protocol versions building on the `org.firebirdsql.gds.ng.wire` package and related packages and classes will need to change their code to correctly take out the "`transmit`"-lock during transmission to the server.
+
+It is possible that this change will not fully address cancellation issues (e.g. we suspect deferred response handling might need additional work, and result sets may need to be invalidated/closed by cancellation);
+this will be subject to further investigation after this thread-safety issue has been addressed.
+
+[appendix]
+== License Notice
+
+The contents of this Documentation are subject to the Public Documentation License Version 1.0 (the “License”);
+you may only use this Documentation if you comply with the terms of this License.
+A copy of the License is available at https://firebirdsql.org/en/public-documentation-license/.
+
+The Original Documentation is "`jdp-2026-01: Additional locking for sending in wire protocol`".
+The Initial Writer of the Original Documentation is Mark Rotteveel, Copyright © 2026.
+All Rights Reserved.
+(Initial Writer contact(s): mark (at) lawinegevaar (dot) nl).
+
+////
+Contributor(s): ______________________________________.
+Portions created by ______ are Copyright © _________ [Insert year(s)].
+All Rights Reserved.
+(Contributor contact(s): ________________ [Insert hyperlink/alias]).
+////
+
+The exact file history is recorded in our Git repository;
+see https://github.com/FirebirdSQL/jaybird