rareMaxim / CloudAPI

Library for working with many API in Delphi
https://t.me/CloudAPI
Other
119 stars 36 forks source link

Access Violation in Deserialize JSON #52

Closed GoLover closed 6 years ago

GoLover commented 7 years ago

i installed this package today and ran the EchoBot demo but when i send /help to bot , most of the time bot is crashed and log : Access violation at address 004080F2 in module 'EchoBot.exe'. Read of address 40878908

i trace the code and find a clue program crashed in function TTelegramBot.ParamsToFormData at line 1837 LTest := dj.From(LParameter.Value, TJsonUtils.DJsonConfig(True)).ToJson; but when i trace more i seen the main problem you used DJSON written by @mauriziodm , in line DJSON.pas in function TdjValueDestination.ToJSON: String; line 885 Result := FParams.GetEngineClass.Serialize(FValue, nil, FParams); crash point is here i just fixed it with change function TdjValueDestination.ToJSON: String; and add sleep(500) to function

function TdjValueDestination.ToJSON: String;
begin
  try
    sleep(500);
    Result := FParams.GetEngineClass.Serialize(FValue, nil, FParams);
  finally
    Self.Free;
  end;
end;
andb24 commented 7 years ago

Hello. "Sleep" as solution looks like a hidden thread synchronization error. By the way, why the code should work better after a pause? Have you encountered any errors with different than 500ms sleep interval? What was the polling interval time set in your echoBot? By default it is = 1000ms.

GoLover commented 7 years ago

i just test with 500ms , and i have no reason for this access violation error , seems like something not created but called in the code !! and polling interval has default value and i didn't change it

andb24 commented 7 years ago

self.free - extremely interesting technique :)

GoLover commented 7 years ago

what did you mean ? do you have better idea ? please correct the code and put it here as best solve for this problem

andb24 commented 7 years ago

I have now idea about it. may be author of code could reveal it.

GoLover commented 7 years ago

do you agree with sleep(500) as a solution ?

andb24 commented 7 years ago

I suppose the reason is somewhere between this JSON and bot library architecture. It is (tglib) have no any thread sync protection. Your solution depends on time intervals so it could fail on heavy or vice versa - light loaded (CPU %) system or on different sizes of data processed by the bot. For example short text or a long article as a message.

GoLover commented 7 years ago

i test different length of text but code works well , anyway only solution for this error is using sleep in DJSON , @mauriziodm we thank you if refine it technicality

mauriziodm commented 7 years ago

Hi to all of you, sorry for the delay of my response but I'm very busy in this time. I sincerely agree with ANDB24 on the analysis of the problem. I do not like Sleep (500) because I think it's an escamotage to solve a problem that is actually elsewhere. The cases are three: 1) "LParameter.Value" does not yet have a valid value at the time of the call; 2) "TJsonUtils.DJsonConfig (True)" does not yet have a valid value at the time of the call (TJsonUtils is not part of DJSON, I do not know what exactly does, even if I imagine it, and especially how does it; 3) a problem due to multithreading programming with unprotected (or unsynchronized) access to resources from multiple threads. Did you try to put Sleep (500) before the call to DJSON? (but anyway would be a nice solution and especially not being the real problem, it could reappear, I think of course). Multithreading programming is very insidious and any problems can be very difficult to debug.

GoLover commented 7 years ago

yes i test using Sleep before DJSON Call , but it doesn't help Sleep isn't good patch but for temporary it's good maybe peoples use this component and had the same problem but they don't comment it as an issue and leave the component, at all your component is very good and useful @mauriziodm thanks for answer

mauriziodm commented 7 years ago

"Self.Free" was needed to implement the fluent interface used for calls to DJSON. I could not use an interface because there are generic methods (methods with a generic parameter) that are incompatible with the interfaces. However, being a fluent interface we do not have a reference to the created object (the TdjValueDestination instance) in some variable so the object must destroy itself when it has finished its task otherwise we would have a memory leak for each call to DJSON. The fluent interface is just an external façade to facilitate and make less complicated and more "elegant" calls to serialization engines and also acts as an abstraction level from them. But perhaps it would be a Self.Dispose instead of Self.Free, the result would not change but would be more appropriate given the multiplatform and multiframework (vcl / fmx) nature of the project. I hope I have explained myself sufficiently why I'm using google translator because my english is a bit poor (apologize to everyone). I remain available to continue the discussion, I will try to answer more quickly.

mauriziodm commented 7 years ago

This phrase "It is (tglib) have no any thread sync protection", written by andb24, is very worrying to me and it may be the cause of your problem. Many people underestimate it. If you can create a little project that replicates the same problem (possibly without external dependencies), send it to me. If no other solution is found, I will add an overload of the TdjValueDestination.ToJson method to the DJSON class to pass an "ADelay" parameter. But I repeat that I do not think it can be a real solution and I would not like the problem to come back to you in the future complicating even more life.

GoLover commented 7 years ago

i test it now with different interval sleep(1) is work fine too it's very interesting @mauriziodm

andb24 commented 7 years ago

about sleep(1) https://habrahabr.ru/post/319402/ This time quant = approx 20ms on Win2000/XP so (by documents) sleep(1)~sleep(20) but sleep call gives the executing quants to other threads for given time period.
So it is (in rough) tell the OS task planner to stop the current thread and choose other thread to run on cpu core. https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx

Something small and fast acting this time in the bot or djson lib and sleep(1) give a chance to it go through.

andb24 commented 7 years ago

Does djson lib use threads, tasks, futures or any other multithreading?

andb24 commented 7 years ago

Bibilo could it be that this code do not know how to serialize the data from params? image

  1. Could you please check in the debugger what data are in the variables FValue and in FParams before the crash?
  2. What happens If you comment sleep and do step-by-step executing of TdjValueDestination.ToJSON func? It can (maybe) force threads to run in other way.  Whether this will lead to the error?
GoLover commented 7 years ago

Function SendMessage -> Result := RequestAPI<TTgMessage>('sendMessage', Parameters); Function RequestAPI -> LTextResponse := SendDataToServer(Method, Parameters); Function SendDataToServer -> LParamToDate := ParamsToFormData(Parameters); Function ParamsToFormData -> LTest := dj.From(LParameter.Value, TJsonUtils.DJsonConfig(True)).ToJson;

and i have to say it's work correctly now , without sleep and i can't understand why !!!

GoLover commented 7 years ago

when i trace it and put BP on LTest and ToJson function it's work correctly !!!

mauriziodm commented 7 years ago

No, DJSON does not use multithreading itself but, often, it is used in multithread contexts. Unfortunately, it is not easy to debug a multithreaded application because even trackpoints do not get true values compared to normal execution. Access to all shared resources between different threads should be protected, in any case.

GoLover commented 7 years ago

FValue

(($A1AF2C, Pointer($A9292C) as IValueData, 0, 11008, 130296576, $7C42B00, TClass($7C42B00), 0, 11008, 130296576, 2.95160754019722e-34, 6.43750619723431e-316, 0.00000000047496e-4933, 130296576, 13029.6576, 130296576, 130296576, ($7C42B00, nil), $7C42B00))

FParams

TdjParams($25AEFC4) as IdjParams

Data values before crash and when i press F8 it's crashed in DJSON Function ToJson ** : It's just when i set BP on ToJson in DJSON

andb24 commented 7 years ago

Lets check something: Set PollingInterval to 10 sec (10 000) and run your app. Afterwars decrease this interval time by 1 sec or 0,5 sec until exception come.

GoLover commented 7 years ago

please test it by yourself and put results here ! i think we should make a breath for code with using sleep() even it's not real solution ! for switching in threads or anything else that i can't understand it !

rareMaxim commented 7 years ago

@bibilo - show me line 65 in file "DJSON/Sources/DJSON.Params.pas"

mauriziodm commented 7 years ago

Dear Bibilo welcome to multithread programming, these are the classic issues when using threads. Also the fact that sometimes it works and sometimes it does not.

GoLover commented 7 years ago

@ms301 image

@mauriziodm i'm a beginner programmer and not professional specially in Delphi, Thanks all of you for paying attention to problem. it's my first experience submitting an issue in GitHub

rareMaxim commented 7 years ago

@bibilo

  1. Delete packages and files of TelegAPI and DJSON.
  2. Install:
andb24 commented 7 years ago

Maybe it is a good time now to sync ms301's DJSON version and mauriziodm's?

mauriziodm commented 7 years ago

Yes, of course, it's time to re-synchronize. My fault that I have not responded to the ms301 PullRequest for some time. I apologize to everyone.

GoLover commented 7 years ago

@ms301 i installed the package you said. but it isn't fixed

andb24 commented 7 years ago

Component propetries, search path for units. replace path to djson sources to external one

rareMaxim commented 7 years ago

@bibilo for fix i need connect to your PC via TeamView. send me login and password in telegram (@rareMax )

mauriziodm commented 7 years ago

Please, after your Teamviewer session report here every results.

rareMaxim commented 6 years ago

@mauriziodm i cant find bug. When I check code in debbuger - exception not fire

mauriziodm commented 6 years ago

I had written some reviews about your last PullRequest a few weeks ago, but it's outdated now and you have to express it explicitly to make it visible. I just wrote it again, if you went to your last PullRequest you should see it.