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

异步保存消息导致异常 #73

Open
FuYan opened this issue Sep 24, 2024 · 8 comments
Open

异步保存消息导致异常 #73

FuYan opened this issue Sep 24, 2024 · 8 comments

Comments

@FuYan
Copy link

FuYan commented Sep 24, 2024

WriteMessageLog

_ = DbContext.SaveChangesAsync()

await DbContext.SaveChangesAsync()

The instance of entity type 'MessageLog' cannot be tracked because another instance with the same key value for {'LogId'} is already being tracked. When attaching existing entities, ensure that only one entity instance with a given key value is attached. Consider using 'DbContextOptionsBuilder.EnableSensitiveDataLogging' to see the conflicting key values.

@dallmann-consulting
Copy link
Owner

I can only assume that two messages are added simultaneously to the log table which creates a problem with the ID generation. Then the solution might be to use "AddAsync(...)" instead of "Add(...)" which calls the DB instantly for an ID:

                        DbContext.MessageLogs.AddAsync(msgLog);
                        Logger.LogTrace("MessageLog => Writing entry '{0}'", message);
                        _ = DbContext.SaveChangesAsync().ContinueWith(task =>

@JeremyMahieu
Do you have other idea?

@JeremyMahieu
Copy link
Contributor

I see... Sorry for helping introduce this bug.

What I think is happening is that two messages with LogId 0 are created locally which would later get a permanent ID in the database. You could check and use Id -1, -2, etc if multiple messages are written.

A deeper cause here is that your DbContext is too longlived. You'll get errors because you are doing two operations at the same time and that is not allowed. So ye adding await does fix it, but at the same takes a slight performance hit.

AddAsync defeats the purpose, you'd still take the database hit, and it's probably not properly configure to do the identiygeneration with addasync, might aswell await then.

Idea1: use id -1, -2, -3 etc if needed. ==> might give DbContext simultanious operations error

MessageLog msgLog = new MessageLog();
(...)
msgLog.LogId = -1*DbContext.MessageLogs.Local.Count();
DbContext.MessageLogs.Add(msgLog);

Idea2: use a temporary DbContext => note I had to borrow the options from the existing context, creating a dbcontext with the constructor like this instead of dependency injection is fishy but doing it properly would lead us much further again.

MessageLog msgLog = new MessageLog
{
    (...)
};
using (var tempDbContext = new OCPPCoreContext(DbContext.Options))
{
  tempDbContext.MessageLogs.Add(msgLog);
  Logger.LogTrace("MessageLog => Writing entry '{0}'", message);
  (...)
}

Idea3: use await and take the performance hit but sleep well at night.

@dallmann-consulting
Copy link
Owner

Yes. I know that the lifetime of the DbContexts is longer than usual. I wasn't aware of this at the beginning of the project but it didn't cause any problems. So I kept it this way. The server should probably generate a new one for every message exchange - like a new browser request. But a clean creation of DbContexts is rather unclear (like your solution borrowing the options instance).

@JeremyMahieu
Copy link
Contributor

JeremyMahieu commented Sep 25, 2024

In long lasting services you inject a IServiceScopeFactory then create a DbContext on demand. This way you don't need the options. You could do that in this case aswell.

using (var scope = _serviceScopeFactory.CreateScope())
{
    // Resolve the DbContext from the scope
    var tempDbContext = scope.ServiceProvider.GetRequiredService<OCPPCoreContext>();

    tempDbContext.MessageLogs.Add(msgLog);
    Logger.LogTrace("MessageLog => Writing entry '{0}'", message);
    (...)
}

@FuYan
Copy link
Author

FuYan commented Sep 26, 2024

This bug is very strange. It only exists for the first message after the websocket connection. As long as the first message is successful, this bug will not occur again. This happens after every connection

@dallmann-consulting
Copy link
Owner

In what scenario? Real chargers? How many? Or only simulator?
The problem hasn't occured in my tests and also not in my little production environment.

@dallmann-consulting
Copy link
Owner

@JeremyMahieu
Thanks for the idea of injecting the service factory. I will probably implement that. But I'm currently busy implementing an Extension/PlugIn mechanism for the server.

@dallmann-consulting
Copy link
Owner

The current code uses a sync save for the messages again. This should avoid the concurrency.
Please check if the problem is solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants