Hi Serguei,
On 3/18/20 6:03 PM, [email protected] wrote:
Hi Patricio,
Good finding, thank you for taking care about this!
The fix looks good in general.
There are several spots with the wrong indent (must be 4, not 2):
64 #define ENTER_CONNECTION(connection) do { \
65 InterlockedIncrement(&connection->refcount); \
66 if (IS_STATE_CLOSED(connection->state)) { \
67 setLastErrorMsg("stream closed"); \
68 InterlockedDecrement(&connection->refcount); \
69 return SYS_ERR; \
70 } \
71 } while (0)
72
73 #define LEAVE_CONNECTION(connection) do { \
74 InterlockedDecrement(&connection->refcount); \
75 } while (0)
I'd also suggest to move content left and use indent 4 from the side.
Done. I already aligned it the same way as STREAM_INVARIANT and I fixed
the indent inside ENTER_CONNECTION().
414 if (*refcount == 0) {
415 sysEventClose(stream->hasData);
416 sysEventClose(stream->hasSpace);
417 sysIPMutexClose(stream->mutex);
418 break;
419 } ...
535 Stream * stream = &connection->outgoing;
536 if (stream->state == STATE_OPEN) {
537 (void)closeStream(stream, JNI_TRUE, &connection->refcount);
538 }
539 stream = &connection->incoming;
540 if (stream->state == STATE_OPEN) {
541 (void)closeStream(stream, JNI_FALSE, &connection->refcount);
542 } ...
551 if (connection->shutdown) {
552 sysEventClose(connection->shutdown);
553 }
554 } ... 1022 shmemBase_sendByte(SharedMemoryConnection *connection,
jbyte data)
1023 {
1024 ENTER_CONNECTION(connection);
1025 jint rc = shmemBase_sendByte_internal(connection, data);
1026 LEAVE_CONNECTION(connection);
1027 return rc;
1028 }
...
1055 jint
1056 shmemBase_receiveByte(SharedMemoryConnection *connection, jbyte
*data)
1057 {
1058 ENTER_CONNECTION(connection);
1059 jint rc = shmemBase_receiveByte_internal(connection, data);
1060 LEAVE_CONNECTION(connection);
1061 return rc;
1062 } ...
1136 jint
1137 shmemBase_sendPacket(SharedMemoryConnection *connection, const
jdwpPacket *packet)
1138 {
1139 ENTER_CONNECTION(connection);
1140 jint rc = shmemBase_sendPacket_internal(connection, packet);
1141 LEAVE_CONNECTION(connection);
1142 return rc;
1143 }
...
1229 jint
1230 shmemBase_receivePacket(SharedMemoryConnection *connection,
jdwpPacket *packet)
1231 {
1232 ENTER_CONNECTION(connection);
1233 jint rc = shmemBase_receivePacket_internal(connection, packet);
1234 LEAVE_CONNECTION(connection);
1235 return rc;
1236 }
Done. Fix all those.
Some other nits were already commented by David and Dan.
I'd suggest to test with tier-5 as well for more safety.
Thanks for looking at this Serguei! I'll give it a new run in mach5 and
add tier5.
Thanks,
Patricio
Thanks,
Serguei
On 3/17/20 13:14, Patricio Chilano wrote:
Hi all,
Please review the following patch:
Bug: https://bugs.openjdk.java.net/browse/JDK-8240902
Webrev: http://cr.openjdk.java.net/~pchilanomate/8240902/v1/webrev/
Calling closeConnection() on an already created/opened connection
includes calls to CloseHandle() on objects that can still be used by
other threads. This can lead to either undefined behavior or, as
detailed in the bug comments, changes of state of unrelated objects.
This issue was found while debugging the reason behind some jshell
test failures seen after pushing 8230594. Not as important, but there
are also calls to closeStream() from createStream()/openStream() when
failing to create/open a stream that will return after executing
"CHECK_ERROR(enterMutex(stream, NULL));" without closing the intended
resources. Then, calling closeConnection() could assert if the reason
of the previous failure was that the stream's mutex failed to be
created/opened. These patch aims to address these issues too.
Tested in mach5 with the current baseline, tiers1-3 and several runs
of open/test/langtools/:tier1 which includes the jshell tests where
this connector is used. I also applied patch
http://cr.openjdk.java.net/~pchilanomate/8240902/triggerbug/webrev
mentioned in the comments of the bug, on top of the baseline and run
the langtool tests with and without this fix. Without the fix running
around 30 repetitions already shows failures in tests
jdk/jshell/FailOverExecutionControlTest.java and
jdk/jshell/FailOverExecutionControlHangingLaunchTest.java. With the
fix I run several hundred runs and saw no failures. Let me know if
there is any additional testing I should do.
As a side note, I see there are a couple of open issues related with
jshell failures (8209848) which could be related to this bug and
therefore might be fixed by this patch.
Thanks,
Patricio
