GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Alternative fix to dead lock challenge by bbakerman · Pull Request #1255 · graphql-java/graphql-java

Conversation

@bbakerman

@bbakerman bbakerman commented Oct 2, 2018

Copy link
Copy Markdown
Member

See #1234

This is similar but more deliberate. That is we work out IF we have to dispatch inside the lock and then do the dispatch outside it.

I think this is a smidge cleaner than the other PR. (but the same effect)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bbakerman added 17 commits June 22, 2018 17:12
@bbakerman
Added @OverRide as part of errorprone code health check
38dfab1
@bbakerman
Revert "Added @OverRide as part of errorprone code health check"
a90a885 
This reverts commit 38dfab1
@bbakerman
Merge remote-tracking branch 'upstream/master'
2653ea0
@bbakerman
Merge remote-tracking branch 'upstream/master'
ead56c4
@bbakerman
Merge remote-tracking branch 'upstream/master'
c893cee
@bbakerman
Merge remote-tracking branch 'upstream/master'
654fd8f
@bbakerman
Merge remote-tracking branch 'upstream/master'
bb2b874
@bbakerman
Merge remote-tracking branch 'upstream/master'
d149b85
@bbakerman
Merge remote-tracking branch 'upstream/master'
e4f451c
@bbakerman
Merge remote-tracking branch 'upstream/master'
79a4df8
@bbakerman
Merge remote-tracking branch 'upstream/master'
b116476
@bbakerman
Merge remote-tracking branch 'upstream/master'
d652315
@bbakerman
Merge remote-tracking branch 'upstream/master'
9e59603
@bbakerman
Merge remote-tracking branch 'upstream/master'
cda28e6
@bbakerman
Merge remote-tracking branch 'upstream/master'
735a989
@bbakerman
Merge remote-tracking branch 'upstream/master'
25ed640
@bbakerman
Brads attempt at graphql-java#1234
75d21b1
@bbakerman
bbakerman requested a review from andimarek October 2, 2018 07:15
@bbakerman
Missed the test
c88ba77
@bbakerman

bbakerman commented Oct 2, 2018

Copy link
Copy Markdown
Member Author

I was able to replicate 100% of the time without the change as per @tinnou original test
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
andimarek
andimarek reviewed Oct 2, 2018
View reviewed changes
Comment thread src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java
public void dispatchIfNotDispatchedBefore(int level, Runnable dispatch) {
public boolean dispatchIfNotDispatchedBefore(int level) {
if (dispatchedLevels.contains(level)) {
Assert.assertShouldNeverHappen("level " + level + " already dispatched");

@andimarek andimarek Oct 2, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

you could actually return the Assert

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@andimarek andimarek added this to the 11.0 milestone Oct 3, 2018
@andimarek andimarek mentioned this pull request Oct 3, 2018
Exploratory fix for deadlock in FieldLevelTrackingApproach #1234
Closed
@bbakerman
bbakerman merged commit 80342b2 into graphql-java:master Oct 3, 2018
@bbakerman bbakerman added the needs to be backported a bugfix that still needs to be backported label Oct 3, 2018
bbakerman added a commit to bbakerman/graphql-java that referenced this pull request Oct 3, 2018
@bbakerman
Alternative fix to dead lock challenge (graphql-java#1255)
7a324f7 
* Added @OverRide as part of errorprone code health check

* Revert "Added @OverRide as part of errorprone code health check"

This reverts commit 38dfab1

* Brads attempt at graphql-java#1234

* Missed the test
@bbakerman bbakerman mentioned this pull request Oct 3, 2018
Stable fix or pr 1255 #1260
Merged
bbakerman added a commit that referenced this pull request Oct 3, 2018
@bbakerman
Alternative fix to dead lock challenge (#1255)
0d80418 
* Added @OverRide as part of errorprone code health check

* Revert "Added @OverRide as part of errorprone code health check"

This reverts commit 38dfab1

* Brads attempt at #1234

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

Reviewers

@andimarek andimarek andimarek left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs to be backported a bugfix that still needs to be backported

Projects

None yet

Milestone

11.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@bbakerman @andimarek
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.