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

.Net: Fix postgres memory store dispose #3177

Merged
merged 7 commits into from
Nov 7, 2023

Conversation

JadynWong
Copy link
Contributor

Motivation and Context

Fix #3121

Description

Convert to IDisposable to execute Dispose.

Contribution Checklist

@JadynWong JadynWong requested a review from a team as a code owner October 14, 2023 04:27
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory connector labels Oct 14, 2023
@JadynWong
Copy link
Contributor Author

JadynWong commented Oct 14, 2023

Create new classlibary with netstandard2.0 framework.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <LangVersion>10</LangVersion>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.SemanticKernel" Version="1.0.0-beta1" />
    <PackageReference Include="Microsoft.SemanticKernel.Connectors.Memory.Postgres" Version="1.0.0-beta1" />
  </ItemGroup>
  
</Project>

Copy the PostgresMemoryStore code into classlibary, naming it MyPostgresMemoryStore.

Reproduce the problem
image

Reslove the problem
image

note

If use MyPostgresMemoryStore directly in console(.net7), that can't reproduce the problem. I can't pinpoint the exact problem, but it seems to be related to TargetFramework. Maybe it should be solved by npgsql.

Reference npgsql/efcore.pg#2834

@RogerBarreto RogerBarreto added the PR: feedback to address Waiting for PR owner to address comments/questions label Oct 19, 2023
@RogerBarreto RogerBarreto added PR: ready to merge PR has been approved by all reviewers, and is ready to merge. and removed PR: feedback to address Waiting for PR owner to address comments/questions labels Oct 24, 2023
@RogerBarreto RogerBarreto added this pull request to the merge queue Nov 7, 2023
Merged via the queue into microsoft:main with commit cc6dd60 Nov 7, 2023
18 checks passed
@JadynWong JadynWong deleted the jadyn/fix-postgres branch November 8, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel memory connector .NET Issue or Pull requests regarding .NET code PR: ready to merge PR has been approved by all reviewers, and is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: System.TypeLoadException Could not load type 'System.Data.Common.DbDataSource' from assembly 'Npgsql'
4 participants