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

recovery ujson error #924

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Zheaoli
Copy link

Zheaoli commented Aug 30, 2017

Sanic use ultra-json as its default json parser, but sometimes when ujson dumps some special dict ,it will raise some Exception such as OverflowError: Invalid Nan value when encoding double , but the standard json library would not . So I suggest to add a mechanism in Sanic that when the ujson.dumps raise Exception , we should use the standard json library and try it again

Zheaoli added some commits Aug 30, 2017

@@ -4,8 +4,9 @@
try:
from ujson import dumps as json_dumps
except:
from json import dumps as json_dumps

from json import dumps as jjson_dumps

This comment has been minimized.

@r0fls

r0fls Aug 31, 2017

Collaborator

You should leave this as json_dumps since otherwise it will raise an exception every time in line 249 when using json instead of ujson.

This comment has been minimized.

@Zheaoli

Zheaoli Aug 31, 2017

OK,I get your means

return HTTPResponse(json_dumps(body, **kwargs), headers=headers,
status=status, content_type=content_type)
try:
return HTTPResponse(json_dumps(body, **kwargs), headers=headers,

This comment has been minimized.

@yunstanford

yunstanford Aug 31, 2017

Member
  1. This line could raise a bunch of exceptions, not only from json_dump. We should not retry the same thing if exception is raised by others. You should either extract json_dump out or catch more specific exception.
    Otherwise, This try/except could be problematic.

  2. If the application'd trigger a exception per request here, this type of handling is very expensive though. It will slow the application a ton. When someone writes the application, they should expect some sort of results, if it expects some results that ujson cannot handle, I'd suggest use HTTPResponse instead of this def json() function, as such, you could use json or simplejson directly instead of this try/except block. That'd be better.

This comment has been minimized.

@Zheaoli

Zheaoli Aug 31, 2017

With no offensive , here's my opinion

  1. ujson with OverflowError: Invalid Nan value when encoding doublehad been issued in 2014, but it's not been fixed , you can look this ujson issue

  2. In Sanic guideline , the first sample is using
    json to decorate the dict that the people want to return . So if there is some people who just start to use Sanic , when they use sample in guideline , there is possibility to raise an Exception because of ujson's bug.

Because of those reason , I think there may have two way to fix this problem

  1. As you say , we can use HTTPResponse instead of json method to avoid this exception , but we have to warn people for that the json method may raise an exception with decorate some special dict because of ujson's bug.

  2. I can commit once more to catch an specific exception and raise other exception

What do you think

This comment has been minimized.

@Zheaoli

This comment has been minimized.

@yunstanford

yunstanford Aug 31, 2017

Member

np, i am just thinking the strategy.
First, i agree, it's ujson issue, thanks for bringing up this discussion, as for fixing,

  1. if you really wanna fix this way, this may be better
try:
    json_resp = json_dumps(body, **kwargs)
except:
    json_resp = jjson_dumps(body, **kwargs)
return HTTPResponse(json_resp, headers=headers,
 +                            status=status, content_type=content_type)
  1. or you can just document the solution above.

I am fine with both. Thanks.
cc: @r0fls @seemethere What do you guys think ?

This comment has been minimized.

@Zheaoli

Zheaoli Aug 31, 2017

I am fine with both too
cc @r0fls @seemethere

This comment has been minimized.

@r0fls

r0fls Aug 31, 2017

Collaborator

Hmm, that makes sense, but if there is no exception when using json instead of ujson, then it's probably ok that it swallows the ujson exception in that case.

This comment has been minimized.

@seemethere

seemethere Aug 31, 2017

Member

@r0fls I think you tagged the wrong person but I'm fine with this the try except approach from @yunstanford

This comment has been minimized.

@Zheaoli

Zheaoli Sep 1, 2017

I‘am ok for both of it. So, who can give me a final decision , and give me five and thumbs up ! Oh, it's a personal issue Hhhhhhh
cc @seemethere , @r0fls , @yunstanford

This comment has been minimized.

@r0fls

r0fls Sep 4, 2017

Collaborator

@Zheaoli the gods have spoken, @yunstanford's method is final.

This comment has been minimized.

@r0fls

r0fls Oct 30, 2017

Collaborator

@Zheaoli do you have time to switch the implementation to the above and resolve the merge conflicts?

@chiuczek

This comment has been minimized.

Copy link

chiuczek commented Nov 1, 2017

Is #948 relevant here? It seems like hiding the json processing from the user isn't the best idea — it might make things look easier, but it doesn't feel right to me. I prefer to have a reasonable default (ujson in this case) but allow it to be changed if needed (so use standard library json).

@r0fls

This comment has been minimized.

Copy link
Collaborator

r0fls commented Nov 3, 2017

Ya, I think @Zheaoli should just use the strategy from #948. I'm going to close this soon unless we hear a strong argument for it.

@Zheaoli Zheaoli closed this Dec 7, 2017

@Zheaoli

This comment has been minimized.

Copy link

Zheaoli commented Dec 7, 2017

@r0fls Sorry about this= =
I closed this PR just now.

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