Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8261391: ZGC crash - SEGV in RevokeOneBias::do_thread #2571

Closed
wants to merge 4 commits into from

Conversation

@robehn
Copy link
Contributor

@robehn robehn commented Feb 15, 2021

Handshakes may contain oop Handles from the requesting thread.
A handshake may be executed by another thread than the requester.
We must make sure any such Handle is safe to use by other threads before executing the handshake.
This change-set adds SWS::start_process() for the requesting thread also (we already do this for the target JavaThread).

Passes t1-5


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261391: ZGC crash - SEGV in RevokeOneBias::do_thread

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2571/head:pull/2571
$ git checkout pull/2571

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 15, 2021

👋 Welcome back rehn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 15, 2021

@robehn The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@robehn robehn marked this pull request as ready for review Feb 15, 2021
@openjdk openjdk bot added the rfr label Feb 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 15, 2021

Webrevs

@fisk
fisk approved these changes Feb 16, 2021
Copy link
Contributor

@fisk fisk left a comment

Looks good. Thanks for fixing!

@openjdk
Copy link

@openjdk openjdk bot commented Feb 16, 2021

@robehn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8261391: ZGC crash - SEGV in RevokeOneBias::do_thread

Reviewed-by: eosterlund, dcubed, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 56 new commits pushed to the master branch:

  • 0c21dd0: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method
  • 2b55501: 8261949: fileStream::readln returns incorrect line string
  • 539c80b: 8261702: ClhsdbFindPC can fail due to PointerFinder incorrectly thinking an address is in a .so
  • 564011c: 8261290: Improve error message for NumberFormatException on null input
  • 18188c2: 8261692: Bugs in clhsdb history support
  • 0825bc5: 8261929: ClhsdbFindPC fails with java.lang.RuntimeException: 'In java stack' missing from stdout/stderr
  • c2509ea: 8261857: serviceability/sa/ClhsdbPrintAll.java failed with "Test ERROR java.lang.RuntimeException: 'cannot be cast to' found in stdout"
  • 2b00367: 8261350: Create implementation for NSAccessibilityCheckBox protocol peer
  • 5a25cea: 8261998: Remove unused shared entry support from utilities/hashtable
  • 4755958: 8262041: javax/xml/jaxp/unittest/common/prettyprint/PrettyPrintTest.java fails after JDK-8260858
  • ... and 46 more: https://git.openjdk.java.net/jdk/compare/84182855307ec8c370aceeaed839c545b7ae1d69...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 16, 2021
@robehn
Copy link
Contributor Author

@robehn robehn commented Feb 16, 2021

Looks good. Thanks for fixing!

Thank you for having a look.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

No code/technical issues with this fix. Just possible naming changes.

src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
if (!_handshakee->is_terminated()) {
StackWatermarkSet::start_processing(_handshakee, StackWatermarkKind::gc);
}
op->prepare(_handshakee, current_thread);

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Feb 16, 2021
Member

Okay, just I'm clear on this fix:

  • You are replacing this single StackWatermarkSet::start_processing call on the handshakee with a call to the new prepare() function.
  • The new prepare() function always does the StackWatermarkSet::start_processing call on the current_target which is the handshakee.
  • The new prepare() function does a StackWatermarkSet::start_processing call on executioner/executer/executing_thread when that thread is different than the requester/handshaker thread.

So you have multiple names for the "target": target, handshakee, and current_target. Is there a reason to create the new name current_target instead of using either target or handshakee that you've used before? Since current_target is new name in this fix, I would like to see that one renamed to either target or handshakee in this fix.

I think you also have multiple names for the "requester": requester and handshaker, but I'm less confused by that naming. In this fix, you're now adding a new name: executioner which could be called executer or executing_thread.

Perhaps you could clean up this naming confusion is follow-up RFE that only fixes naming.

This comment has been minimized.

@robehn

robehn Feb 17, 2021
Author Contributor

1: Request the handshake

  • The thread doing this is the owner of the handshake operation
  • Handles in the handshake operation belongs to this thread and we must do SWS::start_proc() on it

2: Target thread(s)

  • The thread(s) which the operation is performed on
  • We must do SWS::start_proc() on it

3: Executor of the handshake

  • The thread that executes the code that the handshake operation contains.

Handshake operation:
_requester is the thread requested the operation
_target is NULL if we target all threads, otherwise it is the target thread.

The prepare() method is part of the handshake operation and need some additional information.
It needs to know the thread executing the handshake and it needs the current target (since _target may be NULL).

The HandshakeState name for the thread owning the handshake state is handshakee, which is a private implementation detail.
When the handshake state is processing handshakes it calls prepare with _handshakee as current_target and current_thread as parameters to prepare().

target may be NULL
current target may not be NULL
handshakee refers to the handshake state owner (not the target of a handshake operation)

They are thus not interchangeable, but at the moment of execution they can all refer to the same thread.

Regarding handshaker, we have _active_handshaker (same as executioner) inside HandshakeState.
In the context of a handshake operation is is not clear if handshaker would refer to the requester or the executor, therefore I did not use it.

This comment has been minimized.

@robehn

robehn Feb 17, 2021
Author Contributor

I now see that I missed call prepare when executing the handshake by handshakee, fixing!

@robehn
Copy link
Contributor Author

@robehn robehn commented Feb 17, 2021

Update passed t1-3 and locally run the handshake stress tests with ZGC stress options (-XX:+UseZGC -XX:ZCollectionInterval=0.01 -XX:ZFragmentationLimit=0).

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Looks good.

src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
@fisk
fisk approved these changes Feb 17, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Robbin,

I think there is a bug in your fix wrt. async handshakes - see below.

More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

Perhaps @fisk can comment on the general problem.

Thanks,
David

@@ -77,7 +81,7 @@ class AsyncHandshakeOperation : public HandshakeOperation {
jlong _start_time_ns;
public:
AsyncHandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target, jlong start_ns)
: HandshakeOperation(cl, target), _start_time_ns(start_ns) {}
: HandshakeOperation(cl, target, NULL), _start_time_ns(start_ns) {}

This comment has been minimized.

@dholmes-ora

dholmes-ora Feb 18, 2021
Member

Not at all clear why requester is always NULL here. There is obviously a requester thread even in the async case. If that thread created a Handle in the op then don't we need to call start_processing on it?

This comment has been minimized.

@robehn

robehn Feb 18, 2021
Author Contributor

The asynchronous handshake may never be executed, thus having an infinity life-time. (thread never leaves native/blocked)
Referring to a the requesting thread means we must keep it alive forever also.
If you refer to stack-based resources from a thread, means that thread should not touch it's stack before you are done.
If the requester can't use it stack until you are done, you essentially have a synchronous handshake.

The only way we have to keep the requesting thread alive is to put in a ThreadsList, which means we would keep all threads alive potentially forever (until end of VM-life). But still a Handle would be unsafe because it's stack-based.

Therefore it's hard-coded to NULL. (are we suppose to use nullptr now ?)

@@ -305,14 +325,14 @@ void HandshakeOperation::do_handshake(JavaThread* thread) {
}

void Handshake::execute(HandshakeClosure* hs_cl) {
HandshakeOperation cto(hs_cl, NULL);
HandshakeOperation cto(hs_cl, NULL, Thread::current());

This comment has been minimized.

@dholmes-ora

dholmes-ora Feb 18, 2021
Member

It is unfortunate that we need to manifest the current thread here (and elsewhere).

This comment has been minimized.

@robehn

robehn Feb 18, 2021
Author Contributor

Yes, I did that since I did not want to change the API.
I was thinking of:
Handshake::execute(HandshakeClosure* hs_cl, Thread* requester = NULL)
And then go over calls site and see if we have the current thread in a variable.
But I felt that would be better off in another change, if wanted.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 18, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 18/02/2021 5:39 pm, Robbin Ehn wrote:

On Thu, 18 Feb 2021 02:10:56 GMT, David Holmes <dholmes at openjdk.org> wrote:

Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:

Tiny spelling error

src/hotspot/share/runtime/handshake.cpp line 84:

82: public:
83: AsyncHandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target, jlong start_ns)
84: : HandshakeOperation(cl, target, NULL), _start_time_ns(start_ns) {}

Not at all clear why requester is always NULL here. There is obviously a requester thread even in the async case. If that thread created a Handle in the op then don't we need to call start_processing on it?

The asynchronous handshake may never be executed, thus having an infinity life-time. (thread never leaves native/blocked)
Referring to a the requesting thread means we must keep it alive forever also.
If you refer to stack-based resources from a thread, means that thread should not touch it's stack before you are done.
If the requester can't use it stack until you are done, you essentially have a synchronous handshake.
The only way we have to keep the requesting thread alive is to put in a ThreadsList, which means we would keep all threads alive potentially forever (until end of VM-life). But still a Handle would be unsafe because it's stack-based.

I've been confusing Handles with Resources - not realising a Handle is
stack-local and so shouldn't really be shared with another thread. So
are we solving the wrong problem here? Rather than making it "safe" for
one thread to access another's Handle, should we not be using a
different mechanism to pass the oop between threads?

Therefore it's hard-coded to NULL. (are we suppose to use nullptr now ?)

Yes nullptr (when we remember :) )

Thanks,
David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 18, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:

More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not
realising Handles are stack-local to a Thread and can only be shared
with another thread very, very carefully.

David

@robehn
Copy link
Contributor Author

@robehn robehn commented Feb 19, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:

More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not
realising Handles are stack-local to a Thread and can only be shared
with another thread very, very carefully.

Yes, but I do not intended to address if we should disallow passing Handles or if Handles should handle this automatically.
IMHO the right thing would be a 'GlobalHandle' (with oop backing in oopStorage) which you can give owner ship of to another thread.

Are you ok with the change?

David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 19, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 19/02/2021 5:41 pm, Robbin Ehn wrote:

On Thu, 18 Feb 2021 02:19:58 GMT, David Holmes <dholmes at openjdk.org> wrote:

Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:

Tiny spelling error

Hi Robbin,

I think there is a bug in your fix wrt. async handshakes - see below.

More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

Perhaps @fisk can comment on the general problem.

Thanks,
David

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at openjdk.java.net):_

Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:

More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not
realising Handles are stack-local to a Thread and can only be shared
with another thread very, very carefully.

Yes, but I do not intended to address if we should disallow passing Handles or if Handles should handle this automatically.
IMHO the right thing would be a 'GlobalHandle' (with oop backing in oopStorage) which you can give owner ship of to another thread.

Are you ok with the change?

Not really. I think it solves the wrong problem. But I won't block it.

I'll file a RFE to look at the broader problem.

Cheers,
David
-----

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 19, 2021

Mailing list message from Erik Osterlund on hotspot-runtime-dev:

Hi David,

As for the general problem of accessing a private handle from another thread, my thinking was that this could only possibly be safe if a thread ?grabs on to? a remote thread so it?s not concurrently running. Otherwise doing so would never be safe with or without the new concurrent root processing code.
So the reasoning was that if we make it safe for safepoint operations to access any thread?s private oops, and for handshakes to access the handshakee?s private oops, then that should cover that kind of interactions.

However, I did think about the possibility of the handshaker?s private oops being accessed by the handshakee, and thought like you that you just shouldn?t do that. You should use global handles for such shared interactions. But ever since then I have now and then thought ?what if I missed a handle?. And this bug proves that I did indeed miss a handle.

Therefore, while I agree that passing a private handle to be used by another thread is something we shouldn?t do, I would sleep better at night knowing that it is safe to do so anyway, should it happen, in case there is another strange interaction that I missed, or some new code comes in that thinks it?s safe. This is the more robust way of dealing with it.

Thanks,
/Erik

On 18 Feb 2021, at 13:48, David Holmes <david.holmes at oracle.com> wrote:

?Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:
More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not realising Handles are stack-local to a Thread and can only be shared with another thread very, very carefully.

David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Erik,

On 19/02/2021 6:16 pm, Erik Osterlund wrote:

Hi David,

As for the general problem of accessing a private handle from another thread, my thinking was that this could only possibly be safe if a thread ?grabs on to? a remote thread so it?s not concurrently running. Otherwise doing so would never be safe with or without the new concurrent root processing code.

It is safe whenever the two threads use a protocol that ensures safety.
So bundling a local handle into a synchronous VM-op or synchronous
handshake op is safe, because the originator won't move on until the op
has been completed. But any kind of async op would be dangerous - as
would trying to "return" a Handle via the op.

So the reasoning was that if we make it safe for safepoint operations to access any thread?s private oops, and for handshakes to access the handshakee?s private oops, then that should cover that kind of interactions.
However, I did think about the possibility of the handshaker?s private oops being accessed by the handshakee, and thought like you that you just shouldn?t do that. You should use global handles for such shared interactions. But ever since then I have now and then thought ?what if I missed a handle?. And this bug proves that I did indeed miss a handle.

The current case seems to me to be what you would normally encounter.
It is the handshaker that creates the handle and the op and then
potentially has the handshakee execute the op - hence the it has to be
safe for the handshakee to access the handshaker's handles. The reverse
is not expected because, AFAICS, the handshakee never creates the op so
it would only expose local handles if it "returned" them via the op -
and that is inherently unsafe!

Therefore, while I agree that passing a private handle to be used by another thread is something we shouldn?t do, I would sleep better at night knowing that it is safe to do so anyway, should it happen, in case there is another strange interaction that I missed, or some new code comes in that thinks it?s safe. This is the more robust way of dealing with it.

On reflection I can see there are situations where we can construct a
safe exchange of data using handles, so yes this must work. But I don't
like the fact the code performing the exchange has to be aware that
there is some special call that needs to be done to in fact make it safe
though (beyond the protocol) - that is error prone. The Handle logic
should encapsulate this if possible. And ideally we would detect when
such a cross-thread access in in fact unsafe.

So notwithstanding I think there is more to be looked at here, the
current fix is a necessary and desirable one, and I withdraw my
(non-binding) objections.

Thanks,
David
-----

Thanks,
/Erik

On 18 Feb 2021, at 13:48, David Holmes <david.holmes at oracle.com> wrote:

?Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:
More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the service thread or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not realising Handles are stack-local to a Thread and can only be shared with another thread very, very carefully.

David

if (current_target != executing_thread) {
// Only when the target is not executing the handshake itself.
StackWatermarkSet::start_processing(current_target, StackWatermarkKind::gc);
}
Comment on lines +288 to +291

This comment has been minimized.

@dholmes-ora

dholmes-ora Feb 22, 2021
Member

As per my comment to Erik, this situation seems like a programming error to me as there you should not be a way for the target to create a Handle that the executing thread then gets a hold off - data should only flow one way from the requesting thread to the executing thread.

This comment has been minimized.

@robehn

robehn Feb 22, 2021
Author Contributor

StackWatermarkSet::start_processing(current_target, StackWatermarkKind::gc); is for thread local resources which may contain oops. A handshake may read from e.g. the target threads stack, so before the executing thread reads from the stack, we make sure it is safe.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 22/02/2021 5:38 pm, Robbin Ehn wrote:

On Mon, 22 Feb 2021 05:14:39 GMT, David Holmes <dholmes at openjdk.org> wrote:

Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:

Tiny spelling error

src/hotspot/share/runtime/handshake.cpp line 291:

289: // Only when the target is not executing the handshake itself.
290: StackWatermarkSet::start_processing(current_target, StackWatermarkKind::gc);
291: }

As per my comment to Erik, this situation seems like a programming error to me as there you should not be a way for the target to create a Handle that the executing thread then gets a hold off - data should only flow one way from the requesting thread to the executing thread.

StackWatermarkSet::start_processing(current_target, StackWatermarkKind::gc); is for thread local resources which may contain oops. A handshake may read from e.g. the target threads stack, so before the executing thread reads from the stack, we make sure it is safe.

Ah I see - not just to make Handles safe (though they are themselves on
the stack). Thanks for clarifying.

David

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 22, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Correction ...

On 22/02/2021 3:11 pm, David Holmes wrote:

Hi Erik,

On 19/02/2021 6:16 pm, Erik Osterlund wrote:

Hi David,

As for the general problem of accessing a private handle from another
thread, my thinking was that this could only possibly be safe if a
thread ?grabs on to? a remote thread so it?s not concurrently running.
Otherwise doing so would never be safe with or without the new
concurrent root processing code.

It is safe whenever the two threads use a protocol that ensures safety.
So bundling a local handle into a synchronous VM-op or synchronous
handshake op is safe, because the originator won't move on until the op
has been completed. But any kind of async op would be dangerous - as
would trying to "return" a Handle via the op.

So the reasoning was that if we make it safe for safepoint operations
to access any thread?s private oops, and for handshakes to access the
handshakee?s private oops, then that should cover that kind of
interactions.
However, I did think about the possibility of the handshaker?s private
oops being accessed by the handshakee, and thought like you that you
just shouldn?t do that. You should use global handles for such shared
interactions. But ever since then I have now and then thought ?what if
I missed a handle?. And this bug proves that I did indeed miss a handle.

The current case seems to me to be what you would normally encounter.
It is the handshaker that creates the handle and the op and then
potentially has the handshakee execute the op - hence the it has to be
safe for the handshakee to access the handshaker's handles. The reverse
is not expected because, AFAICS, the handshakee never creates the op so
it would only expose local handles if it "returned" them via the op -
and that is inherently unsafe!

Robbin pointed out that the more general issue of safety here involves
e.g walking the stack of the handshakee which may contain oops; it isn't
to per-se make Handles created by the handshakee appear safe.

David
-----

Therefore, while I agree that passing a private handle to be used by
another thread is something we shouldn?t do, I would sleep better at
night knowing that it is safe to do so anyway, should it happen, in
case there is another strange interaction that I missed, or some new
code comes in that thinks it?s safe. This is the more robust way of
dealing with it.

On reflection I can see there are situations where we can construct a
safe exchange of data using handles, so yes this must work. But I don't
like the fact the code performing the exchange has to be aware that
there is some special call that needs to be done to in fact make it safe
though (beyond the protocol) - that is error prone. The Handle logic
should encapsulate this if possible. And ideally we would detect when
such a cross-thread access in in fact unsafe.

So notwithstanding I think there is more to be looked at here, the
current fix is a necessary and desirable one, and I withdraw my
(non-binding) objections.

Thanks,
David
-----

Thanks,
/Erik

On 18 Feb 2021, at 13:48, David Holmes <david.holmes at oracle.com> wrote:

?Correction ...

On 18/02/2021 12:22 pm, David Holmes wrote:
More generally I am concerned by the fact that we have a potential
bug if a Handle created by one thread is accessed by another,
without first calling start_processing() on the creator of the
Handle. This would seem to be error prone to me unless it is
actually hidden away inside the Handle implementation - though how
to do that efficiently is unclear. I'm very concerned that it is too
easy to write code (eg. handing something off to the? service thread
or notification thread) that will be buggy.

I was confusing Handles with Resources in terms of lifecycle - not
realising Handles are stack-local to a Thread and can only be shared
with another thread very, very carefully.

David

@robehn
Copy link
Contributor Author

@robehn robehn commented Feb 22, 2021

/integrate

@openjdk openjdk bot closed this Feb 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 22, 2021

@robehn Since your change was applied there have been 63 commits pushed to the master branch:

  • 5b7b18c: 8259800: timeout in tck test testForkJoin(ForkJoinPool8Test)
  • 419717d: 8261803: Remove unused TaskTerminator in g1 full gc ref proc executor
  • 011f5a5: 8261799: Remove unnecessary cast in psParallelCompact.hpp
  • e9d7c07: 8248318: Remove superfluous use of boxing in ObjectStreamClass
  • 6b7575b: 8228748: Remove GCLocker::_doing_gc
  • c20fb5d: 8261448: Preserve GC stack watermark across safepoints in StackWalk
  • 26c1db9: 8254239: G1ConcurrentMark.hpp unnecessarily disables MSVC++ warning 4522.
  • 0c21dd0: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method
  • 2b55501: 8261949: fileStream::readln returns incorrect line string
  • 539c80b: 8261702: ClhsdbFindPC can fail due to PointerFinder incorrectly thinking an address is in a .so
  • ... and 53 more: https://git.openjdk.java.net/jdk/compare/84182855307ec8c370aceeaed839c545b7ae1d69...master

Your commit was automatically rebased without conflicts.

Pushed as commit d7eebda.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@robehn robehn deleted the robehn:8261391-sws-handshaker branch Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants