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

JSON decoding error parsing stream encoded with Jackson Smile #24198

Closed
eftche opened this issue Dec 12, 2019 · 7 comments
Closed

JSON decoding error parsing stream encoded with Jackson Smile #24198

eftche opened this issue Dec 12, 2019 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@eftche
Copy link

eftche commented Dec 12, 2019

Affects: 5.1.12

With #24009 fixed and versions updated, there's still issues with decoding Smile-encoded stream. Looks like a missyncronization between use of generator and parser:

org.springframework.core.codec.DecodingException: JSON decoding error: Invalid shared name reference 6; 
only got 2 names in buffer (invalid content); nested exception is
com.fasterxml.jackson.core.JsonParseException: Invalid shared name reference 6; only got 2 names in buffer (invalid content)
 at [Source: UNKNOWN; line: -1, column: 6843033]
	at org.springframework.http.codec.json.Jackson2Tokenizer.tokenize(Jackson2Tokenizer.java:99)
	at reactor.core.publisher.FluxMapSignal$FluxMapSignalSubscriber.onNext(FluxMapSignal.java:137)
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 12, 2019
@eftche
Copy link
Author

eftche commented Dec 12, 2019

Test project attached:
test-spring-smile.zip

@poutsma poutsma self-assigned this Dec 13, 2019
@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) for: team-attention and removed for: team-attention labels Dec 13, 2019
@poutsma
Copy link
Contributor

poutsma commented Dec 17, 2019

I'm not sure it's a synchronisation issue. I disabled the USE_THREAD_LOCAL_FOR_BUFFER_RECYCLING feature for Jackson, which has been causing sync issues in the past, but it made no difference.

Maybe it was the "shared name reference" mentioned in the exception that put you on the track of synchronisation, but that is a feature of Smile.

Moreover, I was able to reproduce this issue without using any Spring code, see https://github.com/poutsma/jackson-smile-issue. It does use Reactor, but I suspect the issue is in Jackson itself.

@feutche As the original reporter, could you please report this Jackson issue here, possibly pointing to my repo?

@poutsma poutsma closed this as completed Dec 17, 2019
@poutsma poutsma added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 17, 2019
@poutsma
Copy link
Contributor

poutsma commented Dec 17, 2019

Closed as invalid, as the issue can be reproduced without Spring code.

@eftche
Copy link
Author

eftche commented Dec 18, 2019

Thanks for looking into it. Sorry I wasn't clear as I didn't mean multithreading/concurrency issue but more of maybe inconsistent use of generator and parser so their shared name buffers aren't in sync.

I still think it's the case - in your example (in your repo) - ObjectMapper.writeValuesAsBytes creates new SmileGenerator for every object (!) however there's a single Smile parser created for the whole flow (which is the way it should be, I guess). So obviously their _seenNames buffers are not in sync.
In this case the problem manifests itself when parser buffer overflows and being reset (with expectation that generator did the same, but it didn't, every new generator will only have 2 values in there, name0 and name1). However I've seen cases when backreference is just invalid and Json parser fails trying to assign value to a wrong field (that one drove me mad :), now at least I finally understand where it comes from!).

@eftche
Copy link
Author

eftche commented Dec 18, 2019

So I think this issue should be reopened, the problem is not in Jackson but in the way it's inconsistently used between encoder and decoder.

@poutsma
Copy link
Contributor

poutsma commented Dec 18, 2019

Ok, reopening, but I won't have time to look into this until after my Christmas break.

@poutsma poutsma reopened this Dec 18, 2019
@rstoyanchev rstoyanchev added status: waiting-for-triage An issue we've not yet triaged or decided on and removed status: invalid An issue that we don't feel is valid labels Dec 20, 2019
@poutsma poutsma added this to the 5.2.4 milestone Jan 17, 2020
@poutsma poutsma added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 17, 2020
@poutsma poutsma modified the milestones: 5.2.4, 5.1.14 Jan 17, 2020
@poutsma poutsma added for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x and removed for: team-attention labels Jan 20, 2020
@spring-projects-issues spring-projects-issues removed the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Jan 22, 2020
@poutsma poutsma modified the milestones: 5.1.14, 5.2.4 Jan 22, 2020
@poutsma poutsma added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Jan 22, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels Jan 22, 2020
poutsma added a commit that referenced this issue Jan 22, 2020
Before this commit, the AbstractJackson2Encoder instantiated a
ObjectWriter per value. This is not an issue for single values or
non-streaming scenarios (which effectively are the same, because in the
latter values are collected into a list until offered to Jackson).
However, this does create a problem for SMILE, because it allows for
shared references that do not match up when writing each value with a
new ObjectWriter, resulting in errors parsing the result.

This commit uses Jackson's SequenceWriter for streaming scenarios,
allowing Jackson to reuse the same context for writing multiple values,
fixing the issue described above.

Closes gh-24198
@poutsma
Copy link
Contributor

poutsma commented Jan 22, 2020

Thanks for spotting this. We are now using a Jackson SequenceWriter when streaming, which fixes this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants